Fix various scrolling issues

CHANGES.md

@@ -46,12 +46,14 @@ usual beta standards.
 * Add option to disable IdeaVim in dialogs / single line editors. [VIM-765](https://youtrack.jetbrains.com/issue/VIM-765)  
 Use `set ideaenabledbufs=` to disable IdeaVim in dialog editors.  
 _Note for EAP users: the option name can be changed for the stable release_
+* Reposition cursor when `scrolloff` changes
 
 ### Fixes:
 * [VIM-2150](https://youtrack.jetbrains.com/issue/VIM-2150) `Shift-D` should not delete an empty line
 * [VIM-2157](https://youtrack.jetbrains.com/issue/VIM-2157) Fix tab with an active template
 * [VIM-2156](https://youtrack.jetbrains.com/issue/VIM-2156) Correct up/down motions with inlays
 * [VIM-2144](https://youtrack.jetbrains.com/issue/VIM-2144) Correct text position after block insert with inlays
+* [VIM-2158](https://youtrack.jetbrains.com/issue/VIM-2158) Fix scrolling when `scrolloff` is over half screen height, but less than full height
 
 ## 0.60, 2020-10-09
 

src/com/maddyhome/idea/vim/group/MotionGroup.java

@@ -42,10 +42,12 @@
 import com.maddyhome.idea.vim.handler.TextObjectActionHandler;
 import com.maddyhome.idea.vim.helper.*;
 import com.maddyhome.idea.vim.option.NumberOption;
+import com.maddyhome.idea.vim.option.OptionChangeListener;
 import com.maddyhome.idea.vim.option.OptionsManager;
 import com.maddyhome.idea.vim.ui.ExEntryPanel;
 import kotlin.Pair;
 import kotlin.ranges.IntProgression;
+import org.jetbrains.annotations.Contract;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -184,7 +186,7 @@ private static void moveCaretToView(@NotNull Editor editor) {
     if (caretVisualLine < topVisualLine + scrollOffset) {
       newVisualLine = normalizeVisualLine(editor, topVisualLine + scrollOffset);
     }
-    else if (caretVisualLine >= bottomVisualLine - scrollOffset) {
+    else if (caretVisualLine > bottomVisualLine - scrollOffset) {
       newVisualLine = normalizeVisualLine(editor, bottomVisualLine - scrollOffset);
     }
     else {
@@ -631,26 +633,25 @@ public static void scrollCaretIntoView(@NotNull Editor editor) {
   private static void scrollCaretIntoViewVertically(@NotNull Editor editor, final int caretLine) {
 
     // TODO: Make this work with soft wraps
-    // Vim's algorithm works counts line heights for wrapped lines. We're using visual lines, which handles collapsed
-    // folds, but treats soft wrapped lines as individual lines.
-    // Ironically, after figuring out how Vim's algorithm works (although not *why*), it looks likely to be rewritten as
-    // a dumb line for line reimplementation.
+    // Vim's algorithm works by counting line heights for wrapped lines. We're using visual lines, which handles
+    // collapsed folds, but treats soft wrapped lines as individual lines.
+    // Ironically, after figuring out how Vim's algorithm works (although not *why*) and reimplementing, it looks likely
+    // that this needs to be replaced as a more or less dumb line for line rewrite.
 
     final int topLine = getVisualLineAtTopOfScreen(editor);
     final int bottomLine = getVisualLineAtBottomOfScreen(editor);
 
     // We need the non-normalised value here, so we can handle cases such as so=999 to keep the current line centred
     final int scrollOffset = OptionsManager.INSTANCE.getScrolloff().value();
     final int topBound = topLine + scrollOffset;
-    final int bottomBound = Math.max(topBound + 1, bottomLine - scrollOffset);
+    final int bottomBound = Math.max(topBound, bottomLine - scrollOffset);
 
     // If we need to scroll the current line more than half a screen worth of lines then we just centre the new
     // current line. This mimics vim behavior of e.g. 100G in a 300 line file with a screen size of 25 centering line
     // 100. It also handles so=999 keeping the current line centred.
     // Note that block inlays means that the pixel height we are scrolling can be larger than half the screen, even if
     // the number of lines is less. I'm not sure what impact this has.
     final int height = bottomLine - topLine + 1;
-    final int halfHeight = Math.max(2, (height / 2) - 1);
 
     // Scrolljump isn't handled as you might expect. It is the minimal number of lines to scroll, but that doesn't mean
     // newLine = caretLine +/- MAX(sj, so)
@@ -663,7 +664,7 @@ private static void scrollCaretIntoViewVertically(@NotNull Editor editor, final
     // (See move.c:scroll_cursor_top)
     //
     // When scrolling down (`j` - scrolling window down in the buffer; more lines are visible at the bottom), Vim again
-    // expands lines above and below the new bottom line, but calcualtes things a little differently. The total number
+    // expands lines above and below the new bottom line, but calculates things a little differently. The total number
     // of lines expanded is at least scrolljump and there must be at least scrolloff lines below.
     // Since the lines are advancing simultaneously, it is only possible to get scrolljump/2 above the new cursor line.
     // If there are fewer than scrolljump/2 lines between the current bottom line and the new cursor line, the extra
@@ -678,10 +679,26 @@ private static void scrollCaretIntoViewVertically(@NotNull Editor editor, final
     // out correct scroll locations
     final int scrollJump = getScrollJump(editor, height);
 
-    if (caretLine < topBound) {
+    // Unavoidable fudge value. Multiline rendered doc comments can mean we have very few actual lines, and scrolling
+    // can get stuck in a loop as we re-centre the cursor instead of actually moving it. But if we ignore all inlays
+    // and use the approximate screen height instead of the actual screen height (in lines), we make incorrect
+    // assumptions about the top/bottom line numbers and can scroll to the wrong location. E.g. if there are enough doc
+    // comments (String.java) it's possible to get 12 lines of actual code on screen. Given scrolloff=5, it's very easy
+    // to hit problems, and have (scrolloffset > height / 2) and scroll to the middle of the screen. We'll use this
+    // fudge value to make sure we're working with sensible values. Note that this problem doesn't affect code without
+    // block inlays as positioning the cursor in the middle of the screen always positions it in a deterministic manner,
+    // relative to other text in the file.
+    final int inlayAwareMinHeightFudge = getApproximateScreenHeight(editor) / 2;
+
+    // Note that while these calculations do the same thing that Vim does, it processes them differently. E.g. it
+    // optionally checks and moves the top line, then optionally checks the bottom line. This gives us the same results
+    // via the tests.
+    if (height > inlayAwareMinHeightFudge && scrollOffset > height / 2) {
+      scrollVisualLineToMiddleOfScreen(editor, caretLine);
+    } else if (caretLine < topBound) {
       // Scrolling up, put the cursor at the top of the window (minus scrolloff)
-      // Initial approximation in move.c:update_topline
-      if (topLine + scrollOffset - caretLine >= halfHeight) {
+      // Initial approximation in move.c:update_topline (including same calculation for halfHeight)
+      if (topLine + scrollOffset - caretLine >= Math.max(2, (height / 2) - 1)) {
         scrollVisualLineToMiddleOfScreen(editor, caretLine);
       }
       else {
@@ -692,9 +709,12 @@ private static void scrollCaretIntoViewVertically(@NotNull Editor editor, final
         final int scrollOffsetTopLine = Math.max(0, caretLine - scrollOffset);
         final int newTopLine = Math.min(scrollOffsetTopLine, scrollJumpTopLine);
 
-        // Used is set to the line height of caretLine, and then incremented by line height of the lines above and
-        // below caretLine (up to scrolloff or end of file)
-        final int used = 1 + (newTopLine - topLine) + Math.min(scrollOffset, getVisualLineCount(editor) - topLine);
+        // Used is set to the line height of caretLine (1 or how many lines soft wraps take up), and then incremented by
+        // the line heights of the lines above and below caretLine (up to scrolloff or end of file).
+        // Our implementation ignores soft wrap line heights. Folds already have a line height of 1.
+        final int usedAbove = caretLine - newTopLine;
+        final int usedBelow = Math.min(scrollOffset, getVisualLineCount(editor) - caretLine);
+        final int used = 1 + usedAbove + usedBelow;
         if (used > height) {
           scrollVisualLineToMiddleOfScreen(editor, caretLine);
         }
@@ -1374,4 +1394,21 @@ public int moveCaretToLineEndOffset(@NotNull Editor editor,
       return moveCaretToLineEnd(editor, visualLineToLogicalLine(editor, line), allowPastEnd);
     }
   }
+
+  public static class ScrollOptionsChangeListener implements OptionChangeListener<String> {
+    public static ScrollOptionsChangeListener INSTANCE = new ScrollOptionsChangeListener();
+
+    @Contract(pure = true)
+    private ScrollOptionsChangeListener() {
+    }
+
+    @Override
+    public void valueChange(String oldValue, String newValue) {
+      for (Editor editor : EditorFactory.getInstance().getAllEditors()) {
+        if (UserDataManager.getVimEditorGroup(editor)) {
+          MotionGroup.scrollCaretIntoView(editor);
+        }
+      }
+    }
+  }
 }

src/com/maddyhome/idea/vim/helper/EditorHelper.java

@@ -43,6 +43,12 @@
  * This is a set of helper methods for working with editors. All line and column values are zero based.
  */
 public class EditorHelper {
+  // Set a max height on block inlays to be made visible at the top/bottom of a line when scrolling up/down. This
+  // mitigates the visible area bouncing around too much and even pushing the cursor line off screen with large
+  // multiline rendered doc comments, while still providing some visibility of the block inlay (e.g. Rider's single line
+  // Code Vision)
+  private static final int BLOCK_INLAY_MAX_LINE_HEIGHT = 3;
+
   public static @NotNull Rectangle getVisibleArea(final @NotNull Editor editor) {
     return editor.getScrollingModel().getVisibleAreaOnScrollingFinished();
   }
@@ -201,7 +207,7 @@ public static int normalizeSideScrollOffset(final @NotNull Editor editor, int si
    * @param editor The editor
    * @return The number of screen lines
    */
-  private static int getApproximateScreenHeight(final @NotNull Editor editor) {
+  public static int getApproximateScreenHeight(final @NotNull Editor editor) {
     return getVisibleArea(editor).height / editor.getLineHeight();
   }
 
@@ -641,7 +647,10 @@ public static void scrollVisualLineToCaretLocation(final @NotNull Editor editor,
    * @return Returns true if the window was moved
    */
   public static boolean scrollVisualLineToTopOfScreen(final @NotNull Editor editor, int visualLine) {
-    int y = EditorUtil.getVisualLineAreaStartY(editor, normalizeVisualLine(editor, visualLine));
+
+    final int inlayHeight = EditorUtil.getInlaysHeight(editor, visualLine, true);
+    final int maxInlayHeight = BLOCK_INLAY_MAX_LINE_HEIGHT * editor.getLineHeight();
+    int y = editor.visualLineToY(visualLine) - Math.min(inlayHeight, maxInlayHeight);
 
     // Normalise Y so that we don't try to scroll the editor to a location it can't reach. The editor will handle this,
     // but when we ask for the target location to move the caret to match, we'll get the incorrect value.
@@ -653,7 +662,7 @@ public static boolean scrollVisualLineToTopOfScreen(final @NotNull Editor editor
       // Get the max line number that can sit at the top of the screen
       final int editorHeight = getVisibleArea(editor).height;
       final int virtualSpaceHeight = editor.getSettings().getAdditionalLinesCount() * editor.getLineHeight();
-      final int yLastLine = editor.visualLineToY(EditorHelper.getLineCount(editor));  // last line + 1
+      final int yLastLine = editor.visualLineToY(getLineCount(editor));  // last line + 1
       y = Math.min(y, yLastLine + virtualSpaceHeight - editorHeight);
     }
     return scrollVertically(editor, y);
@@ -662,21 +671,25 @@ public static boolean scrollVisualLineToTopOfScreen(final @NotNull Editor editor
   /**
    * Scrolls the editor to place the given visual line in the middle of the current window.
    *
+   * <p>Snaps the line to the nearest standard line height grid, which gives a good position for both an odd and even
+   * number of lines and mimics what Vim does.</p>
+   *
    * @param editor The editor to scroll
    * @param visualLine The visual line to place in the middle of the current window
    */
   public static void scrollVisualLineToMiddleOfScreen(@NotNull Editor editor, int visualLine) {
-    int y = editor.visualLineToY(normalizeVisualLine(editor, visualLine));
-    int lineHeight = editor.getLineHeight();
-    int height = getVisibleArea(editor).height;
-    scrollVertically(editor, y - ((height - lineHeight) / 2));
+    final int y = editor.visualLineToY(normalizeVisualLine(editor, visualLine));
+    final int screenHeight = getVisibleArea(editor).height;
+    final int lineHeight = editor.getLineHeight();
+    scrollVertically(editor, y - ((screenHeight - lineHeight) / lineHeight / 2 * lineHeight));
   }
 
   /**
    * Scrolls the editor to place the given visual line at the bottom of the screen.
    *
-   * When we're moving the caret down a few lines and want to scroll to keep this visible, we need to be able to place a
-   * line at the bottom of the screen. Due to block inlays, we can't do this by specifying a top line to scroll to.
+   * <p>When we're moving the caret down a few lines and want to scroll to keep this visible, we need to be able to
+   * place a line at the bottom of the screen. Due to block inlays, we can't do this by specifying a top line to scroll
+   * to.</p>
    *
    * @param editor The editor to scroll
    * @param visualLine The visual line to place at the bottom of the current window
@@ -690,7 +703,12 @@ public static boolean scrollVisualLineToBottomOfScreen(@NotNull Editor editor, i
     if (ExEntryPanel.getInstanceWithoutShortcuts().isActive()) {
       exPanelHeight += ExEntryPanel.getInstanceWithoutShortcuts().getHeight();
     }
-    final int y = EditorUtil.getVisualLineAreaEndY(editor, normalizeVisualLine(editor, visualLine)) + exPanelHeight;
+
+    final int normalizedVisualLine = normalizeVisualLine(editor, visualLine);
+    final int lineHeight = editor.getLineHeight();
+    final int inlayHeight = EditorUtil.getInlaysHeight(editor, normalizedVisualLine, false);
+    final int maxInlayHeight = BLOCK_INLAY_MAX_LINE_HEIGHT * lineHeight;
+    final int y = editor.visualLineToY(normalizedVisualLine) + lineHeight + Math.min(inlayHeight, maxInlayHeight) + exPanelHeight;
     final Rectangle visibleArea = getVisibleArea(editor);
     return scrollVertically(editor, max(0, y - visibleArea.height));
   }
@@ -714,19 +732,19 @@ else if (visualColumn > 0) {
     }
 
     final int columnLeftX = editor.visualPositionToXY(new VisualPosition(visualLine, targetVisualColumn)).x;
-    EditorHelper.scrollHorizontally(editor, columnLeftX);
+    scrollHorizontally(editor, columnLeftX);
   }
 
   public static void scrollColumnToMiddleOfScreen(@NotNull Editor editor, int visualLine, int visualColumn) {
     final Point point = editor.visualPositionToXY(new VisualPosition(visualLine, visualColumn));
-    final int screenWidth = EditorHelper.getVisibleArea(editor).width;
+    final int screenWidth = getVisibleArea(editor).width;
 
     // Snap the column to the nearest standard column grid. This positions us nicely if there are an odd or even number
     // of columns. It also works with inline inlays and folds. It is slightly inaccurate for proportional fonts, but is
     // still a good solution. Besides, what kind of monster uses Vim with proportional fonts?
     final int standardColumnWidth = EditorUtil.getPlainSpaceWidth(editor);
     final int x = point.x - (screenWidth / standardColumnWidth / 2 * standardColumnWidth);
-    EditorHelper.scrollHorizontally(editor, x);
+    scrollHorizontally(editor, x);
   }
 
   public static void scrollColumnToRightOfScreen(@NotNull Editor editor, int visualLine, int visualColumn) {
@@ -751,8 +769,8 @@ public static void scrollColumnToRightOfScreen(@NotNull Editor editor, int visua
 
     // Scroll to the left edge of the target column, minus a screenwidth, and adjusted for inlays
     final int targetColumnRightX = editor.visualPositionToXY(new VisualPosition(visualLine, targetVisualColumn + 1)).x;
-    final int screenWidth = EditorHelper.getVisibleArea(editor).width;
-    EditorHelper.scrollHorizontally(editor, targetColumnRightX - screenWidth);
+    final int screenWidth = getVisibleArea(editor).width;
+    scrollHorizontally(editor, targetColumnRightX - screenWidth);
   }
 
   /**

src/com/maddyhome/idea/vim/listener/VimListenerManager.kt

@@ -106,6 +106,7 @@ object VimListenerManager {
 
       OptionsManager.number.addOptionChangeListener(EditorGroup.NumberChangeListener.INSTANCE)
       OptionsManager.relativenumber.addOptionChangeListener(EditorGroup.NumberChangeListener.INSTANCE)
+      OptionsManager.scrolloff.addOptionChangeListener(MotionGroup.ScrollOptionsChangeListener.INSTANCE)
       OptionsManager.showcmd.addOptionChangeListener(ShowCmdOptionChangeListener)
 
       EventFacade.getInstance().addEditorFactoryListener(VimEditorFactoryListener, VimPlugin.getInstance())
@@ -116,6 +117,7 @@ object VimListenerManager {
 
       OptionsManager.number.removeOptionChangeListener(EditorGroup.NumberChangeListener.INSTANCE)
       OptionsManager.relativenumber.removeOptionChangeListener(EditorGroup.NumberChangeListener.INSTANCE)
+      OptionsManager.scrolloff.removeOptionChangeListener(MotionGroup.ScrollOptionsChangeListener.INSTANCE)
       OptionsManager.showcmd.removeOptionChangeListener(ShowCmdOptionChangeListener)
 
       EventFacade.getInstance().removeEditorFactoryListener(VimEditorFactoryListener)

test/org/jetbrains/plugins/ideavim/VimTestCase.kt

@@ -180,26 +180,30 @@ abstract class VimTestCase : UsefulTestCase() {
 
   @JvmOverloads
   protected fun setPositionAndScroll(scrollToLogicalLine: Int, caretLogicalLine: Int, caretLogicalColumn: Int = 0) {
-    val scrolloff = min(OptionsManager.scrolloff.value(), screenHeight / 2)
+
+    // Note that it is possible to request a position which would be invalid under normal Vim!
+    // We disable scrolloff + scrolljump, position as requested, and reset. When resetting scrolloff, Vim will
+    // recalculate the correct offsets, and that could move the top and/or caret line
+    val scrolloff = OptionsManager.scrolloff.value()
     val scrolljump = OptionsManager.scrolljump.value()
+    OptionsManager.scrolloff.set(0)
     OptionsManager.scrolljump.set(1)
 
     // Convert to visual lines to handle any collapsed folds
     val scrollToVisualLine = EditorHelper.logicalLineToVisualLine(myFixture.editor, scrollToLogicalLine)
-    val bottomVisualLine = scrollToVisualLine + screenHeight - 1
+    val bottomVisualLine = scrollToVisualLine + EditorHelper.getApproximateScreenHeight(myFixture.editor) - 1
     val bottomLogicalLine = EditorHelper.visualLineToLogicalLine(myFixture.editor, bottomVisualLine)
 
     // Make sure we're not trying to put caret in an invalid location
-    val boundsTop = EditorHelper.visualLineToLogicalLine(myFixture.editor,
-      if (scrollToVisualLine > scrolloff) scrollToVisualLine + scrolloff else scrollToVisualLine)
-    val boundsBottom = EditorHelper.visualLineToLogicalLine(myFixture.editor,
-      if (bottomVisualLine > EditorHelper.getVisualLineCount(myFixture.editor) - scrolloff - 1) bottomVisualLine - scrolloff else bottomVisualLine)
+    val boundsTop = EditorHelper.visualLineToLogicalLine(myFixture.editor, scrollToVisualLine)
+    val boundsBottom = EditorHelper.visualLineToLogicalLine(myFixture.editor, bottomVisualLine)
     Assert.assertTrue("Caret line $caretLogicalLine not inside legal screen bounds (${boundsTop} - ${boundsBottom})",
       caretLogicalLine in boundsTop..boundsBottom)
 
-    typeText(parseKeys("${scrollToLogicalLine+scrolloff+1}z<CR>", "${caretLogicalLine+1}G", "${caretLogicalColumn+1}|"))
+    typeText(parseKeys("${scrollToLogicalLine+1}z<CR>", "${caretLogicalLine+1}G", "${caretLogicalColumn+1}|"))
 
     OptionsManager.scrolljump.set(scrolljump)
+    OptionsManager.scrolloff.set(scrolloff)
 
     // Make sure we're where we want to be
     assertVisibleArea(scrollToLogicalLine, bottomLogicalLine)