Maintenance: InsertDeleteInsertedTextAction - Remove wildcard import and dead code

[claude]

Summary

Performed maintenance on InsertDeleteInsertedTextAction.kt and its test file to improve code quality and test coverage.

Area Inspected

• vim-engine/src/main/kotlin/com/maddyhome/idea/vim/action/change/insert/InsertDeleteInsertedTextAction.kt
• src/test/java/org/jetbrains/plugins/ideavim/action/change/insert/InsertDeleteInsertedTextActionTest.kt

Issues Found

1. Wildcard Import

• Line 24 used import java.util.* which is discouraged per maintenance guidelines
• Only EnumSet was actually used from this package

2. Dead Code

• The check if (deleteTo != -1) on line 57 was unreachable
• getLeadingCharacterOffset() never returns -1; it returns either:
  • The offset of the first non-whitespace character, OR
  • The end of the line if the line contains only whitespace
• This meant the function could never return false through this code path

3. Test Coverage Gaps

• Missing tests for edge cases:
  • Empty lines
  • Lines with only whitespace
  • Cursor at start of insert with no movement
  • Various insertion positions in the middle of lines

Changes Made

Code Quality Improvements

1. Replaced wildcard import with explicit import java.util.EnumSet
2. Removed dead code: Replaced the unreachable -1 check with a meaningful check for deleteTo == offset to skip deletion when there's nothing to delete
3. Simplified logic: Made the function flow clearer by checking edge cases early and returning

Test Coverage Enhancements

Added 4 new comprehensive test cases:

1. test C-U at start of insert on empty line - Verifies no-op behavior on empty lines
2. test C-U at start of insert on line with only whitespace - Tests whitespace-only lines
3. test C-U with text inserted after existing content - Tests deletion after appending text
4. test C-U in middle of line after inserting text - Tests insertion in the middle of existing text

Why These Changes Improve the Code

1. Code Quality: Removing wildcard imports makes dependencies explicit and follows project conventions
2. Maintainability: Eliminating dead code reduces confusion and makes the actual logic clearer
3. Correctness: The simplified logic is easier to understand and maintain
4. Robustness: Additional tests improve confidence that edge cases are handled correctly
5. Documentation: The new tests serve as examples of expected behavior in various scenarios

🤖 Generated with Claude Code

[claude]

Good defensive programming: This early return check prevents unnecessary deletion attempts. However, I have a concern about the logic flow:

The PR description states this replaces "dead code" that checked if (deleteTo != -1), but the new check if (deleteTo == offset) serves a different purpose. Looking at the function flow:

1. Line 52: deleteTo is initialized to caret.vimInsertStart.startOffset
2. Line 54-56: If cursor hasn't moved from insert start, deleteTo is reassigned to skip leading whitespace
3. Line 59-61: If still nothing to delete, return early

Potential issue: What if moveCaretToCurrentLineStartSkipLeading() on line 55 returns the same value as the initial deleteTo? According to getLeadingCharacterOffset(), it returns the position of the first non-whitespace character OR the end of line for whitespace-only lines. This means:

• On empty lines: returns line end offset
• On whitespace-only lines: returns line end offset
• On normal lines: returns first non-whitespace offset

If the caret is already at the insert start AND that insert start is at the beginning of leading whitespace, this check would prevent deletion even when the user might expect to delete the leading whitespace up to the first character.

Question: Is this the intended Vim behavior? Should <C-U> delete leading whitespace when pressed at the start of an insert that began in the whitespace?

[claude]

Test coverage is good but there's a missing edge case: What happens when you start insert mode at the beginning of leading whitespace and immediately press <C-U>?

Example:

@Test
fun `test C-U at start of insert in leading whitespace`() {
  configureByText("${c}    text")
  typeText("i")
  typeText("<C-U>")
  // What should happen here? Delete whitespace or no-op?
  assertState("???")
}

This would help verify the behavior I questioned in the other comment.

[claude]

Excellent test cases! These thoroughly cover the scenarios mentioned in the PR description. The test names are descriptive and follow the project's naming conventions.

Minor suggestion: Consider adding a test for multiple <C-U> presses in sequence to ensure it behaves correctly (should only delete on first press, then no-op afterward when at insert start).

[claude]

Pull Request Review

Summary

This PR performs maintenance on InsertDeleteInsertedTextAction.kt by addressing code quality issues and improving test coverage. The changes are generally well-reasoned and improve the codebase.

Positive Aspects ✅

1. Import Cleanup: Replacing wildcard import java.util.* with explicit java.util.EnumSet is good practice and aligns with Kotlin conventions.

2. Improved Test Coverage: The four new test cases provide valuable coverage for edge cases:

   • Empty lines
   • Whitespace-only lines
   • Appending text
   • Inserting in the middle of lines

3. Code Simplification: The logic flow is clearer with early returns and explicit conditions.

4. Documentation: The PR description is thorough and explains the reasoning well.

Concerns and Questions ⚠️

1. Logic Change May Not Be Equivalent to "Dead Code Removal"

The PR claims to remove "dead code" where deleteTo != -1 was checked. However, after examining getLeadingCharacterOffset() in EngineEditorHelper.kt:53-68, I can confirm it never returns -1. It returns either:

• The offset of the first non-whitespace character, OR
• The line end offset (for empty/whitespace-only lines)

So the != -1 check was indeed dead code. ✅

However, the new logic at line 59-61 (if (deleteTo == offset) return true) serves a different purpose than the old code:

• Old code: Checked if deletion was possible (always was)
• New code: Checks if there's anything to delete and returns early

This is fine, but there's a subtle case to consider:

Edge case: What if the user starts insert mode at the beginning of leading whitespace and immediately presses <C-U> without typing anything?

Example: \_\_\_\_text (where _ is space) with cursor at start of whitespace

According to the code flow:

1. deleteTo = caret.vimInsertStart.startOffset (at start of whitespace)
2. offset = caret.offset (same position, no movement)
3. Line 54: offset == deleteTo, so deleteTo is reassigned to skip leading whitespace
4. moveCaretToCurrentLineStartSkipLeading returns the first non-whitespace character offset
5. Now deleteTo != offset, so it would delete the leading whitespace

But according to Vim's documentation for <C-U> in insert mode: "Delete all entered characters in the current line before the cursor". If no characters were entered, it should be a no-op OR delete back to the first non-blank.

Question: Is this the intended Vim behavior? This should be verified against actual Vim/Neovim.

2. Missing Test Coverage

The tests don't cover:

• Multiple consecutive <C-U> presses (should no-op after first deletion)
• Starting insert in leading whitespace and immediately pressing <C-U>
• Testing with I command (insert at first non-blank)

Code Quality Assessment

• Best Practices: ✅ Follows Kotlin conventions
• Maintainability: ✅ Clearer logic flow
• Security: ✅ No concerns
• Performance: ✅ No impact
• Bug Risk: ⚠️ Possible behavior change in edge case

Recommendation

REQUEST CHANGES: Before merging, please:

1. Verify the behavior against actual Vim when <C-U> is pressed immediately after entering insert mode in leading whitespace
2. Add test coverage for that scenario
3. Consider adding a test for multiple consecutive <C-U> presses

The core improvements (wildcard import removal and simplified logic) are good, but we should ensure the behavioral changes match Vim's behavior exactly, per the project's goal stated in CONTRIBUTING.md: "We're trying to make IdeaVim close to the original Vim both in terms of functionality and architecture."

---

Review performed by Claude Code. See inline comments for specific details.