Skip to content

Conversation

Eliulm
Copy link
Contributor

@Eliulm Eliulm commented Jan 26, 2023

In essence, the line is calculated by dividing the y-position of the text segment with the cursor by the line height:

var line = Int(textSegmentFrame.maxY / textSegmentFrame.height)

However, this counts the preceding line wraps as lines too. As a result, I have to count the preceding line wraps with:

textLayoutManager.enumerateTextLayoutFragments(from: textLayoutManager.documentRange.location,
                                                           options: [.ensuresLayout, .ensuresExtraLineFragment]
            ) { textLayoutFragment in

                guard let cursorTextLineFragment = textLayoutManager.textLineFragment(at: insertionPointLocation)
                else { return false }

                /// Check whether the textLayoutFragment has line wraps
                if textLayoutFragment.textLineFragments.count > 1 {
                    for lineFragment in textLayoutFragment.textLineFragments {
                        lineWrapsCount += 1
                        /// Do not count lineFragments after the lineFragment where the cursor is placed
                        if lineFragment == cursorTextLineFragment { break }
                    }

                    /// The first lineFragment will be counted as an actual line
                        lineWrapsCount -= 1
                }

                if textLayoutFragment.textLineFragments.contains(cursorTextLineFragment) {
                    return false
                }
                return true
            }

Unfortunately, this does scale with the line count of the file. So we might want to change that if we can come up with a better alternative in the future. As a first implementation, I think it works.

Copy link
Member

@lukepistrol lukepistrol left a comment

Choose a reason for hiding this comment

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

Just a few code style suggestions to comply with our code style guidelines

@thecoolwinter
Copy link
Collaborator

This will break when #131 is fixed. Instead of enumerating the entire string, could we use insertionPointLocation to get the line and column directly? We already compute the visible location, so we'd get the line like: insertionPointLocation.nsRange(...).location - visibleRange.location and then compute the column by walking backwards from the insertionPointLocation until we hit a \n. A similar approach could be used in setCursorPosition.

@Eliulm
Copy link
Contributor Author

Eliulm commented Jan 26, 2023

This will break when #131 is fixed. Instead of enumerating the entire string, could we use insertionPointLocation to get the line and column directly? We already compute the visible location, so we'd get the line like: insertionPointLocation.nsRange(...).location - visibleRange.location and then compute the column by walking backwards from the insertionPointLocation until we hit a \n. A similar approach could be used in setCursorPosition.

Ok, so I guess that I have to wait until the new render method is implemented. I will definitely think about how I can get the updateCursor function to work in that case.

Should I discard this PR?

@thecoolwinter
Copy link
Collaborator

No, don't discard it it can stay open

@thecoolwinter
Copy link
Collaborator

Now that the viewport issue is fixed, it looks like there’s a better way to get the number of lines at the beginning of the visible area.

https://github.com/krzyzanowskim/STTextView/blob/7c378dca8e15902f7762d13bc1e33134a7729ed8/Sources/STTextView/STLineNumberRulerView.swift#L146

Let me know if you’re still willing to work on this, and thank you for being so patient while we got everything else finished.

@Eliulm
Copy link
Contributor Author

Eliulm commented Feb 16, 2023

Not a problem, I was occupied with exams anyways so I did not do too much there. I will work on this in the coming days.

@Eliulm
Copy link
Contributor Author

Eliulm commented Feb 19, 2023

@thecoolwinter So I just reimplemented updateCursorPosition() and it works much better now. However, I am having issues when the last empty line is edited since it is not immediately converted into a new textLineFragment. As a result, the line and column values become out of sync, until the cursor is moved one back:

Screen.Recording.2023-02-19.at.22.04.48.mov

I will see what I can do about it, but I am a bit clueless right now.

@thecoolwinter
Copy link
Collaborator

thecoolwinter commented Feb 20, 2023

Oh that's a weird problem. Good catch though. I'll do some investigating too. Also, great work the changes look good!

@Eliulm
Copy link
Contributor Author

Eliulm commented Mar 5, 2023

@thecoolwinter I have found a workaround for the last line. I had to add a bit more code to get it to work and swiftlint is giving me function_boy_length errors now. Should I ignore it or move the cursor-specific code to a separate new folder?

@austincondiff
Copy link
Collaborator

What is the status of this? It has been open for over a month now and I want to make sure we aren't blocked.

@Eliulm
Copy link
Contributor Author

Eliulm commented Mar 15, 2023

It is working as expected. I just need to know, whether I should allocate all the cursor logic to a separate file/folder since I am getting SwiftLint function_boy_length errors now.

@thecoolwinter
Copy link
Collaborator

So sorry I missed the comments on this. You’ll have to move it to a new file. You can make a Controller folder and move STTextViewController.swift into there, and make a new STTextViewController+Cursor.swift file for the cursor additions.

@Eliulm
Copy link
Contributor Author

Eliulm commented Mar 15, 2023

No Problem, I will do it shortly.

@Eliulm
Copy link
Contributor Author

Eliulm commented Mar 15, 2023

@thecoolwinter Alright, I moved it over.

@Eliulm Eliulm requested a review from thecoolwinter March 15, 2023 20:52
Copy link
Collaborator

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for all the work on this!

@thecoolwinter
Copy link
Collaborator

Just waiting for @lukepistrol to review b/c I can't merge the branch until all requested changes are resolved.

@lukepistrol lukepistrol merged commit 0e3ca03 into CodeEditApp:main Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants