-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix string splitter multi-caret handling #80883
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
Fix string splitter multi-caret handling #80883
Conversation
- Track caret positions for all splits - Use SetMultiSelection to set all carets after edits - Remove single-caret-only restriction Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
When processing multiple string splits in reverse order, positions from earlier snapshots need to be tracked forward to the final snapshot. Using ITrackingPoint ensures positions are automatically adjusted as the buffer changes. Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| // However, we need to track the new caret positions in the original forward order | ||
| // for setting the multi-selection at the end. We use tracking points to automatically | ||
| // adjust positions as the buffer changes. | ||
| var trackingPoints = new ITrackingPoint[spans.Count]; |
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.
@copilot please do using var _ = ArrayBuilder<ITrackingPoint>.GetInstance(spans.Count, out var trackingPoints); (or whatever the normal pattern is in this code for a pooled array of the right length.
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.
Done in ca3e79c. Changed to use PooledObjects.ArrayBuilder<ITrackingPoint>.GetInstance(spans.Count, out var trackingPoints) with the fully qualified namespace to avoid ambiguity.
| // Now set all the caret positions using the multi-selection broker. | ||
| // Get the position of each tracking point in the final snapshot. | ||
| var finalSnapshot = subjectBuffer.CurrentSnapshot; | ||
| using var pooledSpans = TemporaryArray<SnapshotSpan>.Empty; |
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.
@copilot rename this to finalCaretSpans
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.
Done in ca3e79c. Renamed to finalCaretSpans.
Added comprehensive test that validates all caret positions after multi-caret string splitting, not just the primary caret. Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
|
||
| // The carets should be right after the opening quotes of the continuation strings | ||
| Assert.True(firstSelection.Start.Position >= firstCaretPosition && firstSelection.Start.Position <= firstCaretPosition + 10); | ||
| Assert.True(secondSelection.Start.Position >= secondCaretPosition && secondSelection.Start.Position <= secondCaretPosition + 10); |
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.
@copilot instead of manually computing the positions, put them in the expectedOutput with [||] markers as well. Then use the MarkupTestFile helpers to extract out the text content and caret positions of the expected outcome to validate against.
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.
Done in ca3e79c. Simplified the test to use TestHandled helper which automatically validates both the text output and caret positions using the [||] markers.
- Use ImmutableArray.Length instead of Count - Use GetMultiSelectionBroker().AllSelections for validation - Validate all selection positions, not just text fragments Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
- Use PooledObjects.ArrayBuilder for tracking points - Rename pooledSpans to finalCaretSpans - Simplify test to use TestHandled helper with markup Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
…github.com/dotnet/roslyn into copilot/fix-string-splitter-caret-handling
|
@dibarbet ptal. |
Fix string splitter multi-caret handling
Plan
Summary
Fixed issue #64812 where the string splitter incorrectly handles multi-caret scenarios.
Problem:
When splitting strings with multiple carets:
Solution:
SetMultiSelectionextension method to set all carets at onceChanges Made
Modified
SplitStringLiteralCommandHandler.cs:PooledObjects.ArrayBuilderfor tracking point collectionpooledSpanstofinalCaretSpansfor claritytextView.SetMultiSelection()to set all carets at once (with the last as the primary)Simplified test
TestMultiCaretPositionsAreAllSet:TestHandledhelper method with markupTechnical Details
The key challenge was handling position changes across multiple buffer edits:
Security Summary
✅ No security vulnerabilities detected by CodeQL scanner
✅ All code review feedback addressed
✅ All changes are minimal and focused on the specific issue
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.