unicode: Handle wide grapheme clusters that start with narrow code point#1
Closed
jacobsandlund wants to merge 9 commits intographeme-breakfrom
Closed
unicode: Handle wide grapheme clusters that start with narrow code point#1jacobsandlund wants to merge 9 commits intographeme-breakfrom
jacobsandlund wants to merge 9 commits intographeme-breakfrom
Conversation
Merged
2 tasks
Owner
Author
|
Actual PR ghostty-org#10465 |
jacobsandlund
pushed a commit
that referenced
this pull request
Feb 19, 2026
This PR introduces unit tests and a supporting Mock NSView for testing the SplitTree implementation in Swift. It includes 51 tests which achieve approximately 93.13% (949/1019) coverage of SplitTree.swift's branches. <details> <summary>Coverage</summary> <pre> ./ghostty/macos/Sources/Features/Splits/SplitTree.swift 93.13% (949/1019) SplitTree.Path.isEmpty.getter 100.00% (1/1) SplitTree.isEmpty.getter 100.00% (3/3) SplitTree.isSplit.getter 100.00% (3/3) SplitTree.init() 100.00% (3/3) SplitTree.init(view:) 100.00% (3/3) SplitTree.contains(_:) 100.00% (4/4) SplitTree.inserting(view:at:direction:) 100.00% (6/6) SplitTree.find(id:) 100.00% (4/4) SplitTree.removing(_:) 93.75% (15/16) SplitTree.replacing(node:with:) 93.75% (15/16) SplitTree.focusTarget(for:from:) 82.14% (46/56) closure #1 in SplitTree.focusTarget(for:from:) 100.00% (1/1) closure ghostty-org#2 in SplitTree.focusTarget(for:from:) 100.00% (1/1) closure ghostty-org#3 in SplitTree.focusTarget(for:from:) 100.00% (3/3) implicit closure #1 in SplitTree.focusTarget(for:from:) 0.00% (0/1) SplitTree.equalized() 100.00% (5/5) SplitTree.resizing(node:by:in:with:) 92.00% (69/75) closure #1 in SplitTree.resizing(node:by:in:with:) 100.00% (1/1) SplitTree.viewBounds() 100.00% (4/4) SplitTree.init(from:) 76.00% (19/25) SplitTree.encode(to:) 100.00% (15/15) SplitTree.Node.find(id:) 100.00% (13/13) SplitTree.Node.node(view:) 88.89% (16/18) SplitTree.Node.path(to:) 100.00% (32/32) search #1 <A>(_:) in SplitTree.Node.path(to:) 100.00% (27/27) SplitTree.Node.node(at:) 89.47% (17/19) SplitTree.Node.inserting(view:at:direction:) 86.84% (33/38) SplitTree.Node.replacingNode(at:with:) 100.00% (43/43) replaceInner #1 <A>(current:pathOffset:) in SplitTree.Node.replacingNode(at:with:) 96.67% (29/30) SplitTree.Node.remove(_:) 70.27% (26/37) implicit closure #1 in SplitTree.Node.remove(_:) 100.00% (1/1) SplitTree.Node.resizing(to:) 100.00% (16/16) SplitTree.Node.leftmostLeaf() 87.50% (7/8) SplitTree.Node.rightmostLeaf() 87.50% (7/8) SplitTree.Node.equalize() 100.00% (4/4) SplitTree.Node.equalizeWithWeight() 100.00% (30/30) SplitTree.Node.weightForDirection(_:) 83.33% (10/12) SplitTree.Node.calculateViewBounds(in:) 100.00% (50/50) SplitTree.Node.viewBounds() 100.00% (26/26) SplitTree.Node.spatial(within:) 100.00% (18/18) SplitTree.Node.dimensions() 80.77% (21/26) SplitTree.Node.spatialSlots(in:) 100.00% (53/53) SplitTree.Spatial.slots(in:from:) 100.00% (47/47) closure #1 in SplitTree.Spatial.slots(in:from:) 100.00% (1/1) distance #1 <A>(from:to:) in SplitTree.Spatial.slots(in:from:) 100.00% (6/6) closure ghostty-org#2 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) implicit closure #1 in closure ghostty-org#2 in SplitTree.Spatial.slots(in:from:) 100.00% (1/1) closure ghostty-org#3 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) closure ghostty-org#4 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) implicit closure #1 in closure ghostty-org#4 in SplitTree.Spatial.slots(in:from:) 100.00% (1/1) closure ghostty-org#5 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) closure ghostty-org#6 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) implicit closure #1 in closure ghostty-org#6 in SplitTree.Spatial.slots(in:from:) 100.00% (1/1) closure ghostty-org#7 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) closure ghostty-org#8 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) implicit closure #1 in closure ghostty-org#8 in SplitTree.Spatial.slots(in:from:) 100.00% (1/1) closure ghostty-org#9 in SplitTree.Spatial.slots(in:from:) 100.00% (3/3) SplitTree.Spatial.doesBorder(side:from:) 100.00% (20/20) closure #1 in SplitTree.Spatial.doesBorder(side:from:) 100.00% (1/1) closure ghostty-org#2 in SplitTree.Spatial.doesBorder(side:from:) 100.00% (3/3) static SplitTree.Node.== infix(_:_:) 100.00% (13/13) SplitTree.Node.init(from:) 66.67% (12/18) SplitTree.Node.encode(to:) 100.00% (11/11) SplitTree.Node.leaves() 100.00% (9/9) SplitTree.makeIterator() 100.00% (3/3) implicit closure #1 in SplitTree.makeIterator() 100.00% (1/1) SplitTree.Node.makeIterator() 0.00% (0/3) SplitTree.startIndex.getter 100.00% (3/3) SplitTree.endIndex.getter 100.00% (3/3) implicit closure #1 in SplitTree.endIndex.getter 100.00% (1/1) SplitTree.subscript.getter 100.00% (5/5) implicit closure #1 in SplitTree.subscript.getter 100.00% (1/1) implicit closure ghostty-org#2 in implicit closure #1 in SplitTree.subscript.getter 100.00% (1/1) implicit closure ghostty-org#3 in SplitTree.subscript.getter 0.00% (0/1) implicit closure ghostty-org#4 in SplitTree.subscript.getter 0.00% (0/1) SplitTree.index(after:) 100.00% (4/4) implicit closure #1 in SplitTree.index(after:) 100.00% (1/1) implicit closure ghostty-org#2 in SplitTree.index(after:) 0.00% (0/1) SplitTree.Node.structuralIdentity.getter 100.00% (3/3) SplitTree.Node.StructuralIdentity.init(_:) 100.00% (3/3) static SplitTree.Node.StructuralIdentity.== infix(_:_:) 100.00% (3/3) SplitTree.Node.StructuralIdentity.hash(into:) 100.00% (3/3) SplitTree.Node.isStructurallyEqual(to:) 100.00% (18/18) implicit closure #1 in SplitTree.Node.isStructurallyEqual(to:) 100.00% (1/1) implicit closure ghostty-org#2 in SplitTree.Node.isStructurallyEqual(to:) 100.00% (1/1) SplitTree.Node.hashStructure(into:) 100.00% (14/14) SplitTree.structuralIdentity.getter 100.00% (3/3) SplitTree.StructuralIdentity.init(_:) 100.00% (4/4) static SplitTree.StructuralIdentity.== infix(_:_:) 100.00% (4/4) implicit closure #1 in static SplitTree.StructuralIdentity.== infix(_:_:) 100.00% (1/1) SplitTree.StructuralIdentity.hash(into:) 80.00% (8/10) static SplitTree.StructuralIdentity.areNodesStructurallyEqual(_:_:) 90.00% (9/10) </pre> </details> I chose this as a good place to start contributing to Ghostty because I was curious about the macOS implementation, and there was a specific request for help with testing (ghostty-org#7879). My process for writing the tests was basically reading [SplitTree.swift](./macos/Sources/Features/Splits/SplitTree.swift) to understand it, then writing tests for each high-level method and checking against code coverage to capture all the code paths: ## Running ```bash rm -rf /tmp/ghostty-test.xcresult xcodebuild -project macos/Ghostty.xcodeproj \ -scheme GhosttyTest \ -configuration Debug \ test \ -destination 'platform=macOS' \ -enableCodeCoverage YES \ -resultBundlePath /tmp/ghostty-test.xcresult \ -only-testing:GhosttyTests/SplitTreeTests \ 2>&1 | xcbeautify ``` ## Coverage ```bash xcrun xccov view --report /tmp/ghostty-test.xcresult | grep 'SplitTree\.' ``` This was originally implemented in [~38 commits](https://github.com/pouwerkerk/ghostty/pull/1/commits), but I squashed them down to 1 commit for easier review. ## AI Disclosure The tests were written by me, but I used Opus 4.6 to explain some parts of the code, and then finally to provide feedback on the tests. It suggested tests for `nodeStructuralIdentityInSet` and `nodeStructuralIdentityDistinguishesLeaves` as well as [the Parameterized test](pouwerkerk@6a0bca4), `resizingAdjustsRatio`, which seemed like a clever way to collapse 12 individual tests into 3 parameterized ones that still run 12 cases total. I didn't know this feature existed, and it seems like a great way to write tests that are more maintainable. I read this relatively new feature in the [Swift Docs](https://developer.apple.com/documentation/testing/parameterizedtesting). I find this to be a particularly useful feature of Claude/related agents, where it can suggest better ways of writing something in a more idiomatic way, and it taught me something new, which is always fun. I'm more than happy to continue work on tests for ghostty-org#7879 and always welcome to any feedback you have.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR follows follows ghostty-org#9680.
This PR updates the logic in Terminal
printto include more cases of changing a cell to be wide due to a grapheme cluster that needs to be wide but starts off narrow. The existing case of this is a text-presentation code point followed by VS16 to make it emoji presentation. This PS handles more cases that are found in scripts such as Devanagari where the correct grapheme width calculation sums up multiple code points of non-zero widths. An example, as seen from uucode's issue #1 isक्ष, which after ghostty-org#9680 is now one grapheme cluster instead of two, but the U+0915 (first code point) is width one and U+0937 (final code point) is also width one, and the whole cluster should be width 1 + 1 = 2. This is important to address with the grapheme break change otherwise these scripts would show with narrow cells, incorrectly.This also adds tests to make sure moving the cell is handled correctly, which was not the case for the existing VS16 logic.
There's a lot of code here to handle transferring the graphemes when the narrow cell should wrap to the next line to become wide. I'd like feedback on the approach here before attempting to clean anything up (pull it out into a separate method?).
AI was used in some of the uucode changes in ghostty-org#9678 (Amp--primarily for tests), but everything was carefully vetted and much of it done by hand. This PR was made without AI.