Skip to content

Commit

Permalink
composer: Make text editor more robust
Browse files Browse the repository at this point in the history 
This change makes the text editor/composer more robust and simple and solves Github issue #1558

It builds on the changes made for #1211, and on Jericho's (Jericho Hasselbush <[email protected]>) discovery during his work on #1544

It uses setContentCompressionResistance, disabled text box scrolling, and dynamic height adjustments (based on more accurate layout calculations) to allow several improvements:
- It ensures lines get wrapped and not overflown
- It uses system native "scroll cursor into view" when typing, eliminating the need to a ghost caret
- It ensures we do not have a scroll view within a scroll view (which is confusing)
- It ensures that we set the height of the text box to its ideal value using a native layout calculation (Removes some issues with copying and pasting larger text)
- It resolves other small issues, such as #1558

Issue #1558 repro
-----------------

Result: VERIFIED

Device: iPhone 14 Pro Simulator
iOS: 17.0
Damus: `476f52562a70c2615ad084640dd1a0ba5c4c12e3`

Issue #1558 steps:

1. Type "hello world, hello @da"
2. Select "Damus" in contact list
3. Try moving cursor to the end of "world". Cursor should have gone there, but it immediately goes back to the end of "@Damus " instead.

Testing for #1558
-----------------

Result: PASS

Device: iPhone 14 Pro Simulator
iOS: 17.0
Damus: This commit
Steps:

1. Type "hello world, hello @da"
2. Select "Damus" in contact list
3. Try moving cursor to the end of "world". Cursor goes there.

General functionality testing
-----------------------------

Result: CONDITIONAL PASS.
Summary: Behaviour is improved from #1211 patch, and #1558 is fixed. There are a few remaining issues, but they do not look like regressions from these changes. More details below.

Device: iPhone 14 Pro Simulator
iOS: 17.0
Damus: This commit
Coverage:

1. Basic typing works. PASS
2. Basic user tagging works. PASS
3. Typing long text line wraps the line. PASS
4. Adding newlines to the end of the text works and text is visible (i.e. Text box is expanding with text). PASS
5. Adding lots of newlines causes the text box and inner PostView content to expand, and those contents can be scrolled. PASS
6. Typing text when cursor is out of view (both up and down) causes PostView to scroll the cursor into view. PASS
7. Tagging user on a line positioned at the middle of the screen causes view to scroll cursor into view. PASS
8. Tagging user on a very long line positioned causes view to scroll cursor into view. PASS
9. Pasting very long text (5 paragraphs of Lorem Ipsum) expands the text box as necessary, wraps all long lines, scrolls cursor at the end into view. PASS
10. Scrolling through very long text shows that there is only one scroll view active (PostView's). PASS
11. Typing text that expands text box does not cause jitters. PASS
12. Typing mentions do not cause jitter. PASS
13. Adding newline from the end of a mid paragraph unfortunately still causes cursor to jump to the end of the text. This is an existing bug (#1521). EXISTING ISSUE.
14. Tagging a user at the end of a line when there are other lines below it may cause the cursor to jump a few characters forward. It is unclear whether this is a regression because prior to this change the cursor would get stuck at the end of the mention. But since this is a very specific edge case that might not be a regression, it might be a good idea to address this on a separate ticket. CONDITIONAL PASS
15. Could not run PostView unit tests due to various build errors on the test target.

Closes: #1558
Changelog-Fixed: Fix situations where the note composer cursor gets stuck in one place after tagging a user
Changelog-Fixed: Fix some note composer issues, such as when copying/pasting larger text, and make the post composer more robust.
Signed-off-by: Daniel D’Aquino <[email protected]>
Signed-off-by: William Casarin <[email protected]>
  • Loading branch information
@danieldaquino @jb55
danieldaquino authored and jb55 committed Oct 2, 2023
1 parent 89acde1 commit 1150a14
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 76 deletions.
37 changes: 0 additions & 37 deletions damus/Views/PostView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ struct PostView: View {
@State var filtered_pubkeys: Set<Pubkey> = []
@State var focusWordAttributes: (String?, NSRange?) = (nil, nil)
@State var newCursorIndex: Int?
@State var caretRect: CGRect = CGRectNull
@State var textHeight: CGFloat? = nil

@State var mediaToUpload: MediaUpload? = nil
Expand Down Expand Up @@ -220,13 +219,6 @@ struct PostView: View {
self.newCursorIndex = nil
}, updateCursorPosition: { newCursorIndex in
self.newCursorIndex = newCursorIndex
}, onCaretRectChange: { uiView in
// When the caret position changes, we change the `caretRect` in our state, so that our ghost caret will follow our caret
if let selectedStartRange = uiView.selectedTextRange?.start {
DispatchQueue.main.async {
caretRect = uiView.caretRect(for: selectedStartRange)
}
}
})
.environmentObject(tagModel)
.focused($focus)
Expand Down Expand Up @@ -316,9 +308,6 @@ struct PostView: View {

func Editor(deviceSize: GeometryProxy) -> some View {
HStack(alignment: .top, spacing: 0) {
if(caretRect != CGRectNull) {
GhostCaret
}
VStack(alignment: .leading, spacing: 0) {
HStack(alignment: .top) {
ProfilePicView(pubkey: damus_state.pubkey, size: PFP_SIZE, highlight: .none, profiles: damus_state.profiles, disable_animation: damus_state.settings.disable_animation)
Expand All @@ -340,25 +329,6 @@ struct PostView: View {
}
}

// The GhostCaret is a vertical projection of the editor's caret that should sit beside the editor.
// The purpose of this view is create a reference point that we can scroll our ScrollView into
// This is necessary as a bridge to communicate between:
// - The UIKit-based UITextView (which has the caret position)
// - and the SwiftUI-based ScrollView/ScrollReader (where scrolling commands can only be done via the SwiftUI "ID" parameter
var GhostCaret: some View {
Rectangle()
.foregroundStyle(DEBUG_SHOW_GHOST_CARET_VIEW ? .cyan : .init(red: 0, green: 0, blue: 0, opacity: 0))
.frame(
width: DEBUG_SHOW_GHOST_CARET_VIEW ? caretRect.width : 0,
height: caretRect.height)
// Use padding to vertically align our ghost caret with our actual text caret.
// Note: Programmatic scrolling cannot be done with the `.position` modifier.
// Experiments revealed that the scroller ignores the position modifier.
.padding(.top, caretRect.origin.y)
.id(GHOST_CARET_VIEW_ID)
.disabled(true)
}

func fill_target_content(target: PostTarget) {
self.post = initialString()
self.tagModel.diff = post.string.count
Expand Down Expand Up @@ -396,13 +366,6 @@ struct PostView: View {
.onAppear {
scroll_to_event(scroller: scroller, id: "post", delay: 1.0, animate: true, anchor: .top)
}
// Note: The scroll commands below are specific because there seems to be quirk with ScrollReader where sending it to the exact same position twice resets its scroll position.
.onChange(of: caretRect.origin.y, perform: { newValue in
scroller.scrollTo(GHOST_CARET_VIEW_ID)
})
.onChange(of: searchingIsNil, perform: { newValue in
scroller.scrollTo(GHOST_CARET_VIEW_ID)
})
}

// This if-block observes @ for tagging
Expand Down
63 changes: 24 additions & 39 deletions damus/Views/TextViewWrapper.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@

import SwiftUI

// Defines how much extra bottom spacing will be applied after the text.
// This will avoid jitters when applying new lines, by ensuring it has enough space until the height is updated on the next view update cycle
let TEXT_BOX_BOTTOM_MARGIN_OFFSET: CGFloat = 30.0

struct TextViewWrapper: UIViewRepresentable {
@Binding var attributedText: NSMutableAttributedString
@EnvironmentObject var tagModel: TagModel
Expand All @@ -19,19 +15,16 @@ struct TextViewWrapper: UIViewRepresentable {
let cursorIndex: Int?
var getFocusWordForMention: ((String?, NSRange?) -> Void)? = nil
let updateCursorPosition: ((Int) -> Void)
let onCaretRectChange: ((UITextView) -> Void)

func makeUIView(context: Context) -> UITextView {
let textView = UITextView()
textView.delegate = context.coordinator

// Scroll has to be enabled. When this is disabled, the text input will overflow horizontally, even when its frame's width is limited.
textView.isScrollEnabled = true
// However, a scrolling text box inside of its parent scrollview does not provide a very good experience. We should have the textbox expand vertically
// To simulate that the text box can expand vertically, we will listen to text changes and dynamically change the text box height in response.
// Add an observer so that we can adapt the height of the text input whenever the text changes.
textView.addObserver(context.coordinator, forKeyPath: "contentSize", options: .new, context: nil)
textView.showsVerticalScrollIndicator = false
// Disable scrolling (this view will expand vertically as needed to fit text)
textView.isScrollEnabled = false
// Set low content compression resistance to make this view wrap lines of text, and avoid text overflowing to the right
textView.setContentCompressionResistancePriority(.defaultLow, for: .horizontal)
textView.setContentCompressionResistancePriority(.required, for: .vertical)

TextViewWrapper.setTextProperties(textView)
return textView
Expand All @@ -52,9 +45,26 @@ struct TextViewWrapper: UIViewRepresentable {
setCursorPosition(textView: uiView)
let range = uiView.selectedRange

// Set the text height that will fit all the text
// This is needed because the UIKit auto-layout prefers to overflow the text to the right than to expand the text box vertically, even with low horizontal compression resistance
self.setIdealHeight(uiView: uiView)

uiView.selectedRange = NSRange(location: range.location + tagModel.diff, length: range.length)
tagModel.diff = 0
}

/// Based on our desired layout, calculate the ideal size of the text box, then set the height to the ideal size
private func setIdealHeight(uiView: UITextView) {
DispatchQueue.main.async { // Queue on main thread, because modifying view state directly during re-render causes undefined behavior
let idealSize = uiView.sizeThatFits(CGSize(
width: uiView.frame.width, // We want to stay within the horizontal bounds given to us
height: .infinity // We can expand vertically without any resistance
))
if self.textHeight != idealSize.height { // Only update height when it changes, to avoid infinite re-render calls
self.textHeight = idealSize.height
}
}
}

private func setCursorPosition(textView: UITextView) {
guard let index = cursorIndex, let newPosition = textView.position(from: textView.beginningOfDocument, offset: index) else {
Expand All @@ -64,38 +74,27 @@ struct TextViewWrapper: UIViewRepresentable {
}

func makeCoordinator() -> Coordinator {
Coordinator(attributedText: $attributedText, getFocusWordForMention: getFocusWordForMention, updateCursorPosition: updateCursorPosition, onCaretRectChange: onCaretRectChange, textHeight: $textHeight)
Coordinator(attributedText: $attributedText, getFocusWordForMention: getFocusWordForMention, updateCursorPosition: updateCursorPosition)
}

class Coordinator: NSObject, UITextViewDelegate {
@Binding var attributedText: NSMutableAttributedString
var getFocusWordForMention: ((String?, NSRange?) -> Void)? = nil
let updateCursorPosition: ((Int) -> Void)
let onCaretRectChange: ((UITextView) -> Void)
@Binding var textHeight: CGFloat?

init(attributedText: Binding<NSMutableAttributedString>,
getFocusWordForMention: ((String?, NSRange?) -> Void)?,
updateCursorPosition: @escaping ((Int) -> Void),
onCaretRectChange: @escaping ((UITextView) -> Void),
textHeight: Binding<CGFloat?>
updateCursorPosition: @escaping ((Int) -> Void)
) {
_attributedText = attributedText
self.getFocusWordForMention = getFocusWordForMention
self.updateCursorPosition = updateCursorPosition
self.onCaretRectChange = onCaretRectChange
_textHeight = textHeight
}

func textViewDidChange(_ textView: UITextView) {
attributedText = NSMutableAttributedString(attributedString: textView.attributedText)
processFocusedWordForMention(textView: textView)
}

func textViewDidChangeSelection(_ textView: UITextView) {
textView.scrollRangeToVisible(textView.selectedRange)
onCaretRectChange(textView)
}

private func processFocusedWordForMention(textView: UITextView) {
var val: (String?, NSRange?) = (nil, nil)
Expand Down Expand Up @@ -183,20 +182,6 @@ struct TextViewWrapper: UIViewRepresentable {
}
}

override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) {
if keyPath == "contentSize", let textView = object as? UITextView {
DispatchQueue.main.async {
// Update text view height when text content size changes to fit all text content
// This is necessary to avoid having a scrolling text box combined with its parent scrolling view
self.updateTextViewHeight(textView: textView)
}
}
}

func updateTextViewHeight(textView: UITextView) {
self.textHeight = textView.contentSize.height + TEXT_BOX_BOTTOM_MARGIN_OFFSET
}

}
}

0 comments on commit 1150a14

Please sign in to comment.