handle logic to auto close and delete brackets#501
handle logic to auto close and delete brackets#501lunargon wants to merge 4 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
|
Could this feature be made optional? For me this is one of the most annoying features in other editors, frequently causing me to write things like Some more general remarks:
|
|
@Consolatis You can try my ver first. Because delete only auto delete if between that is empty.
My auto just delete with not thing between open and close. Can you check that first. If still annoy for you I will add some change to make that to optional or remove that feature. |
|
I'm not entirely sure about merging this logic just yet without having a settings model allowing users to turn this behavior off. I'll continue to let this PR sit for a while longer but try to keep it in the back of my mind. |
|
@lhecker If needed, I’ll update with the settings ( from project) later. I see that my PR is currently out of date with the main branch. |
karan68
left a comment
There was a problem hiding this comment.
The_Checker Review — RequiresReview
5 entities changed: 0 critical, 1 high, 3 medium, 1 low
Must-Review Entities
| Entity | File | Risk | Classification | Dependents |
|---|---|---|---|---|
| impl::textarea_handle_input | src/tui.rs | 0.52 High | DocFunctional | 2 |
| impl::should_delete_both_brackets | src/tui.rs | 0.46 Medium | Added | 2 |
| impl::handle_auto_closing_brackets | src/tui.rs | 0.46 Medium | Added | 2 |
| impl::delete_both_brackets | src/tui.rs | 0.46 Medium | Added | 2 |
Analysis: 1140ms (diff: 547ms, graph: 555ms, scoring: 36ms)
💬 impl::textarea_handle_input — Review cannot be completed because the specific changes are not visible. The function textarea_handle_input grew by 24 lines, but the additions are not shown in the provided truncated Before/After sections. The diff content is required to evaluate the high-risk (0.52) changes involving input handling, bracket deletion, and auto-closing logic. (1 issues)
💬 impl::should_delete_both_brackets — Implements bracket pair detection logic correctly for auto-delete functionality, but requires unnecessary mutable references that constrain the API. Also needs protection against negative cursor coordinates when casting to usize for the buffer read operations. (2 issues)
❌ impl::handle_auto_closing_brackets — Critical correctness issues: byte-length used for character-position cursor adjustment breaks non-ASCII brackets, and missing bounds check risks underflow panic. Also ignores I/O errors and lacks atomicity. (4 issues)
💬 impl::delete_both_brackets — Helper function assumes cursor is positioned between brackets without validation. Consider adding checks to verify the cursor is bracketed by matching delimiters before deletion, or document preconditions clearly. Also verify TextBuffer::delete handles boundary conditions gracefully to avoid partial deletions. (3 issues)
| /// Checks if we should delete both opening and closing brackets | ||
| /// | ||
| /// Returns true if we should delete both brackets, false for normal deletion | ||
| fn should_delete_both_brackets(&mut self, tb: &mut TextBuffer) -> bool { |
There was a problem hiding this comment.
💬 impl::should_delete_both_brackets
Implements bracket pair detection logic correctly for auto-delete functionality, but requires unnecessary mutable references that constrain the API. Also needs protection against negative cursor coordinates when casting to usize for the buffer read operations.
- medium: Function signature uses &mut self and &mut TextBuffer but performs no mutation; should use &self and &TextBuffer instead. Unnecessary mutable references restrict API usage and prevent calling this predicate function when only immutable access is available
- medium: Casting cursor_pos.x to usize without bounds checking risks integer overflow if cursor_pos.x is a signed type (e.g., i32) and negative. This could cause read_backward/read_forward to panic or behave incorrectly with an extremely large offset value
|
|
||
| /// Handles auto-closing brackets by inserting both opening and closing brackets | ||
| /// and positioning the cursor between them. | ||
| fn handle_auto_closing_brackets(&mut self, tb: &mut TextBuffer, opening: &str, closing: &str) { |
There was a problem hiding this comment.
❌ impl::handle_auto_closing_brackets
Critical correctness issues: byte-length used for character-position cursor adjustment breaks non-ASCII brackets, and missing bounds check risks underflow panic. Also ignores I/O errors and lacks atomicity.
- high: Byte/character mismatch:
closing.len()returns byte length, but cursor logical positions (CoordType) typically measure Unicode scalar values or display columns. For multi-byte UTF-8 closing brackets (e.g., Unicode '」' is 3 bytes, 1 char), subtracting byte length moves cursor too far left, creating invalid or wrong positions. Useclosing.chars().count()instead ofclosing.len(). - high: Integer underflow risk: If the cursor x-position is smaller than
closing.len()(e.g., at start of line),new_offset.x - closing.len() as CoordTypepanics in debug mode or wraps in release mode for unsignedCoordType. Must verifynew_offset.x >= char_countbefore subtraction. - medium: Errors silently ignored:
tb.write_raw()likely returns aResultindicating buffer full or I/O failures, but the code discards it with;. On write failure, the cursor still moves to an incorrect calculated position, leaving the buffer in an inconsistent state. - low: Non-atomic operation: If the opening bracket write succeeds but the closing bracket write fails (or cursor move fails), the buffer ends up with an unmatched opening bracket without the corresponding closing bracket. Consider grouping changes in a transaction or undo block.
| } | ||
|
|
||
| /// Deletes both opening and closing brackets when cursor is between them | ||
| fn delete_both_brackets(&mut self, tb: &mut TextBuffer) { |
There was a problem hiding this comment.
💬 impl::delete_both_brackets
Helper function assumes cursor is positioned between brackets without validation. Consider adding checks to verify the cursor is bracketed by matching delimiters before deletion, or document preconditions clearly. Also verify TextBuffer::delete handles boundary conditions gracefully to avoid partial deletions.
- medium: No validation that characters being deleted are actually brackets. If the cursor is not positioned between matching brackets (e.g., 'ab|cd'), this will delete arbitrary characters 'b' and 'c' instead of failing safely.
- medium: Potential boundary issues: if the cursor is at the end of the buffer, the forward delete may be a no-op (depending on TextBuffer implementation), causing only the backward deletion to execute, or vice versa if at the start. This leaves the text buffer in an inconsistent state with only one character deleted.
- low: Operation is not atomic. If the first delete succeeds but the second panics or fails, the buffer remains with only one bracket deleted, requiring manual cleanup.
Handle Auto Close Parentheses, Braces, Brackets
To solved #488
I add logic to handle:
Auto close: If you type '(' or '[' or '{' with have auto close that and cursor will go between these.Auto delete:For example: You have (|) ( this is parentheses with cursor between, if you delete - type
BACKboth open and close will be delele)