-
Notifications
You must be signed in to change notification settings - Fork 809
Fix VIM-2656: Incorrect shift motion behavior with tabs #1416
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| /* | ||
| * Copyright 2003-2024 The IdeaVim authors | ||
| * | ||
| * Use of this source code is governed by an MIT-style | ||
| * license that can be found in the LICENSE.txt file or at | ||
| * https://opensource.org/licenses/MIT. | ||
| */ | ||
|
|
||
| package org.jetbrains.plugins.ideavim.action.change.shift | ||
|
|
||
| import com.intellij.application.options.CodeStyle | ||
| import com.intellij.ide.highlighter.JavaFileType | ||
| import org.jetbrains.plugins.ideavim.SkipNeovimReason | ||
| import org.jetbrains.plugins.ideavim.TestWithoutNeovim | ||
| import org.jetbrains.plugins.ideavim.VimJavaTestCase | ||
| import org.junit.jupiter.api.Test | ||
|
|
||
| /** | ||
| * Tests for VIM-2656: Incorrect shift motion behavior with tabs | ||
| */ | ||
| class ShiftRightTabsTest : VimJavaTestCase() { | ||
| @TestWithoutNeovim(SkipNeovimReason.NOT_VIM_TESTING) | ||
| @Test | ||
| fun `test shift right with tabs should add full tab`() { | ||
| val before = """ | ||
| |public class C { | ||
| |.${c}println("hello"); | ||
| |} | ||
| """.trimMargin().dotToTab() | ||
|
|
||
| val after = """ | ||
| |public class C { | ||
| |..println("hello"); | ||
| |} | ||
| """.trimMargin().dotToTab() | ||
|
|
||
| usingTabs { | ||
| configureByJavaText(before) | ||
| typeText(">>") | ||
| assertState(after) | ||
| } | ||
| } | ||
|
|
||
| @TestWithoutNeovim(SkipNeovimReason.NOT_VIM_TESTING) | ||
| @Test | ||
| fun `test shift right with tabs from no indent`() { | ||
| val before = """ | ||
| |public class C { | ||
| |${c}println("hello"); | ||
| |} | ||
| """.trimMargin() | ||
|
|
||
| val after = """ | ||
| |public class C { | ||
| |.println("hello"); | ||
| |} | ||
| """.trimMargin().dotToTab() | ||
|
|
||
| usingTabs { | ||
| configureByJavaText(before) | ||
| typeText(">>") | ||
| assertState(after) | ||
| } | ||
| } | ||
|
|
||
| @TestWithoutNeovim(SkipNeovimReason.NOT_VIM_TESTING) | ||
| @Test | ||
| fun `test shift right multiple times with tabs`() { | ||
| val before = """ | ||
| |public class C { | ||
| |${c}println("hello"); | ||
| |} | ||
| """.trimMargin() | ||
|
|
||
| val after = """ | ||
| |public class C { | ||
| |...println("hello"); | ||
| |} | ||
| """.trimMargin().dotToTab() | ||
|
|
||
| usingTabs { | ||
| configureByJavaText(before) | ||
| typeText(">>".repeat(3)) | ||
| assertState(after) | ||
| } | ||
| } | ||
|
|
||
| @TestWithoutNeovim(SkipNeovimReason.NOT_VIM_TESTING) | ||
| @Test | ||
| fun `test shift right with existing misaligned spaces should use full tabs`() { | ||
| // VIM-2656: Starting with 2 spaces, >> should result in full tab indent, not mixed | ||
| val before = """ | ||
| |public class C { | ||
| | ${c}println("hello"); | ||
| |} | ||
| """.trimMargin() | ||
|
|
||
| val after = """ | ||
| |public class C { | ||
| |.println("hello"); | ||
| |} | ||
| """.trimMargin().dotToTab() | ||
|
|
||
| usingTabs { | ||
| configureByJavaText(before) | ||
| typeText(">>") | ||
| assertState(after) | ||
| } | ||
| } | ||
|
|
||
| private fun usingTabs(action: () -> Unit) { | ||
| val testSettings = CodeStyle.createTestSettings() | ||
| val javaSettings = testSettings.getIndentOptions(JavaFileType.INSTANCE) | ||
| javaSettings.USE_TAB_CHARACTER = true | ||
| javaSettings.TAB_SIZE = 4 | ||
| javaSettings.INDENT_SIZE = 4 | ||
|
Comment on lines
+115
to
+116
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage note: All tests use This configuration is valid and used in some codebases where you want 4-space indent levels but tabs render as 8 spaces. |
||
| CodeStyle.setTemporarySettings(fixture.project, testSettings) | ||
| try { | ||
| action() | ||
| } finally { | ||
| CodeStyle.dropTemporarySettings(fixture.project) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1998,15 +1998,18 @@ abstract class VimChangeGroupBase : VimChangeGroup { | |
| } | ||
| } else { | ||
| // Shift non-blockwise selection | ||
| val indentSize = indentConfig.getIndentSize(1) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good practice: Moving the |
||
| for (l in sline..eline) { | ||
| val soff = editor.getLineStartOffset(l) | ||
| val eoff = editor.getLineEndOffset(l, true) | ||
| val woff = injector.motion.moveCaretToLineStartSkipLeading(editor, l) | ||
| val col = editor.offsetToBufferPosition(woff).column | ||
| val limit = max(0.0, (col + dir * indentConfig.getIndentSize(count)).toDouble()) | ||
| .toInt() | ||
| // Calculate the current indent depth (floor division) | ||
| val currentDepth = col / indentSize | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Integer division here will always floor the result, which is correct for the fix. However, could you add a brief comment explaining why we use integer division (floor) rather than rounding? This would help future maintainers understand that misaligned indents should round down to the previous indent level before shifting. Example: With |
||
| // Calculate the new depth after shifting | ||
| val newDepth = max(0, currentDepth + dir * count) | ||
| if (col > 0 || soff != eoff) { | ||
| val indent = indentConfig.createIndentBySize(limit) | ||
| val indent = indentConfig.createIndentByDepth(newDepth) | ||
| replaceText(editor, caret, soff, woff, indent) | ||
| } | ||
| } | ||
|
|
||
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.
Excellent test: This is the key test case that demonstrates the bug fix. Starting with 2 spaces of misaligned indent and expecting it to round to 1 full tab (not 1 tab + 2 spaces) is exactly what VIM-2656 was about.
Suggestion for additional edge case: Consider adding a test where
TAB_SIZE != INDENT_SIZE(e.g.,TAB_SIZE=8, INDENT_SIZE=4). This is a valid configuration and would verify the fix works correctly when indent depth and tab rendering differ.