Avoid recentering when user says so

Avoid recentering when user says so

[Eli Zaretskii]

This is related to bug #6671, read there if you want to know the gory
details.

In a nutshell, Emacs currently doesn't honor meticulously enough user
settings such as scroll-conservatively and scroll-up/down-aggressively.
Sometimes, especially if redisplay cannot keep up with high-rate
keyboard input, it falls back on recentering, which these variables
are supposed to avoid.  The last attempt to solve this introduced a
bug that is the subject of bug #6671.

Following the latest discussions in that bug report, I came up with a
way to solve this problem without incurring unduly slow movement far
away in the buffer.  The patch below seems to give good results, but
as I don't use the above settings (I like the default recentering),
I'm not a good candidate for testing the patch.

Would people who use these variables please try the patch below, and
report any findings, both positive and negative?  Please report your
findings to the bug tracker at 6671@debbugs.gnu.org, so that the
details get filed there.

Given positive feedback, I will commit this to the trunk.

Thanks.

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2011-03-26 12:20:20 +0000
+++ src/xdisp.c	2011-03-26 18:07:58 +0000
@@ -13003,6 +13003,9 @@ enum
   SCROLLING_NEED_LARGER_MATRICES
 };
 
+/* If scroll-conservatively is more than this, never recenter.  */
+#define SCROLL_LIMIT 100
+
 static int
 try_scrolling (Lisp_Object window, int just_this_one_p,
 	       EMACS_INT arg_scroll_conservatively, EMACS_INT scroll_step,
@@ -13016,7 +13019,8 @@ try_scrolling (Lisp_Object window, int j
   int dy = 0, amount_to_scroll = 0, scroll_down_p = 0;
   int extra_scroll_margin_lines = last_line_misfit ? 1 : 0;
   Lisp_Object aggressive;
-  int scroll_limit = INT_MAX / FRAME_LINE_HEIGHT (f);
+  /* We will never try scrolling more than this number of lines.  */
+  int scroll_limit = SCROLL_LIMIT;
 
 #if GLYPH_DEBUG
   debug_method_add (w, "try_scrolling");
@@ -13032,14 +13036,14 @@ try_scrolling (Lisp_Object window, int j
   else
     this_scroll_margin = 0;
 
-  /* Force arg_scroll_conservatively to have a reasonable value, to avoid
-     overflow while computing how much to scroll.  Note that the user
-     can supply scroll-conservatively equal to `most-positive-fixnum',
-     which can be larger than INT_MAX.  */
+  /* Force arg_scroll_conservatively to have a reasonable value, to
+     avoid scrolling too far away with slow move_it_* functions.  Note
+     that the user can supply scroll-conservatively equal to
+     `most-positive-fixnum', which can be larger than INT_MAX.  */
   if (arg_scroll_conservatively > scroll_limit)
     {
-      arg_scroll_conservatively = scroll_limit;
-      scroll_max = INT_MAX;
+      arg_scroll_conservatively = scroll_limit + 1;
+      scroll_max = scroll_limit * FRAME_LINE_HEIGHT (f);
     }
   else if (scroll_step || arg_scroll_conservatively || temp_scroll_step)
     /* Compute how much we should try to scroll maximally to bring
@@ -13076,7 +13080,7 @@ try_scrolling (Lisp_Object window, int j
 	  /* Compute how many pixels below window bottom to stop searching
 	     for PT.  This avoids costly search for PT that is far away if
 	     the user limited scrolling by a small number of lines, but
-	     always finds PT if arg_scroll_conservatively is set to a large
+	     always finds PT if scroll_conservatively is set to a large
 	     number, such as most-positive-fixnum.  */
 	  int slack = max (scroll_max, 10 * FRAME_LINE_HEIGHT (f));
 	  int y_to_move =
@@ -13128,12 +13132,12 @@ try_scrolling (Lisp_Object window, int j
 	return SCROLLING_FAILED;
 
       start_display (&it, w, startp);
-      if (scroll_max < INT_MAX)
+      if (arg_scroll_conservatively <= scroll_limit)
 	move_it_vertically (&it, amount_to_scroll);
       else
 	{
 	  /* Extra precision for users who set scroll-conservatively
-	     to most-positive-fixnum: make sure the amount we scroll
+	     to a large number: make sure the amount we scroll
 	     the window start is never less than amount_to_scroll,
 	     which was computed as distance from window bottom to
 	     point.  This matters when lines at window top and lines
@@ -14201,11 +14205,10 @@ redisplay_window (Lisp_Object window, in
 	}
     }
 
-  /* Finally, just choose place to start which centers point */
+  /* Finally, just choose a place to start which positions point
+     according to user preferences.  */
 
  recenter:
-  if (centering_position < 0)
-    centering_position = window_box_height (w) / 2;
 
 #if GLYPH_DEBUG
   debug_method_add (w, "recenter");
@@ -14217,10 +14220,62 @@ redisplay_window (Lisp_Object window, in
   if (!buffer_unchanged_p)
     w->base_line_number = Qnil;
 
-  /* Move backward half the height of the window.  */
+  /* Determine the window start relative to point.  */
   init_iterator (&it, w, PT, PT_BYTE, NULL, DEFAULT_FACE_ID);
   it.current_y = it.last_visible_y;
+  if (centering_position < 0)
+    {
+      int margin =
+	scroll_margin > 0
+	? min (scroll_margin, WINDOW_TOTAL_LINES (w) / 4)
+	: 0;
+      int scrolling_up = PT > CHARPOS (startp) + margin;
+      Lisp_Object aggressive =
+	scrolling_up
+	? BVAR (current_buffer, scroll_up_aggressively)
+	: BVAR (current_buffer, scroll_down_aggressively);
+
+      if (!MINI_WINDOW_P (w)
+	  && scroll_conservatively > SCROLL_LIMIT || NUMBERP (aggressive))
+	{
+	  int pt_offset = 0;
+
+	  /* Setting scroll-conservatively overrides
+	     scroll-*-aggressively.  */
+	  if (!scroll_conservatively && NUMBERP (aggressive))
+	    {
+	      double float_amount = XFLOATINT (aggressive);
+
+	      pt_offset = float_amount * WINDOW_BOX_TEXT_HEIGHT (w);
+	      if (pt_offset == 0 && float_amount > 0)
+		pt_offset = 1;
+	      if (pt_offset)
+		margin -= 1;
+	    }
+	  /* Compute how much to move the window start backward from
+	     point so that point will be displayed where the user
+	     wants it.  */
+	  if (scrolling_up)
+	    {
+	      centering_position = it.last_visible_y;
+	      if (pt_offset)
+		centering_position -= pt_offset;
+	      centering_position -=
+		FRAME_LINE_HEIGHT (f) * (1 + margin + (last_line_misfit != 0));
+	      /* Don't let point enter the scroll margin near top of
+		 the window.  */
+	      if (centering_position < margin * FRAME_LINE_HEIGHT (f))
+		centering_position = margin * FRAME_LINE_HEIGHT (f);
+	    }
+	  else
+	    centering_position = margin * FRAME_LINE_HEIGHT (f) + pt_offset;
+	}
+      else
+	/* Move backward from point half the height of the window.  */
+	centering_position = window_box_height (w) / 2;
+    }
   move_it_vertically_backward (&it, centering_position);
+
   xassert (IT_CHARPOS (it) >= BEGV);
 
   /* The function move_it_vertically_backward may move over more
@@ -14237,8 +14292,9 @@ redisplay_window (Lisp_Object window, in
 
   it.current_x = it.hpos = 0;
 
-  /* Set startp here explicitly in case that helps avoid an infinite loop
-     in case the window-scroll-functions functions get errors.  */
+  /* Set the window start position here explicitly, to avoid an
+     infinite loop in case the functions in window-scroll-functions
+     get errors.  */
   set_marker_both (w->start, Qnil, IT_CHARPOS (it), IT_BYTEPOS (it));
 
   /* Run scroll hooks.  */

Re: Avoid recentering when user says so

[Eli Zaretskii]

> Date: Sat, 26 Mar 2011 20:48:35 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Following the latest discussions in that bug report, I came up with a
> way to solve this problem without incurring unduly slow movement far
> away in the buffer.  The patch below seems to give good results

Btw, in the discussions related to bug #6671, some people said that
Emacs should pay attention to the value of another variable,
scroll-preserve-screen-position, when Emacs decides where to put point
after moving it to a far away location.

I would like to point out that this option has nothing to do with the
issue at hand, although its name might suggest that it does.  This
option is only examined by the scroll-up and scroll-down commands,
which are just one way of scrolling.  No other command ever looks at
this option.  (Documentation seem to suggest that any command that has
non-nil scroll-command property will also behave according to this
option, but unless I'm missing something, I cannot see how that could
be true.)

Therefore, the patch I proposed does not consult this option at all.

Re: Avoid recentering when user says so

[Juanma Barranquero]

On Sat, Mar 26, 2011 at 20:52, Eli Zaretskii <eliz@gnu.org> wrote:

> Btw, in the discussions related to bug #6671, some people said that
> Emacs should pay attention to the value of another variable,
> scroll-preserve-screen-position, when Emacs decides where to put point
> after moving it to a far away location.

He, not exactly ;-)

Chong asked if the unwanted recentering affected other scrolling
commands, and I said that scroll-(up|down)-command do preserve the
screen position and don't do any recentering.

I think the documentation makes reasonably clear that
`scroll-preserve-screen-position' is not defined for non-scrolling
movements (like goto-line), and as per line-by-line scrolling
(assuming you've got track-eol == t) you can already combine
pgup/pgdown and up/down and get the cursor back to the same screen
position, so it's well defined and well-behaved. It's true that
(next|previous)-line do not really obey
scroll-preserve-screen-position, but as they try to move vertically
and are smart with respect to end of line (with track-eol), the net
effect is the same.

    Juanma

Re: Avoid recentering when user says so

[Eli Zaretskii]

> Date: Sat, 26 Mar 2011 20:48:35 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> +      if (!MINI_WINDOW_P (w)
> +	  && scroll_conservatively > SCROLL_LIMIT || NUMBERP (aggressive))

Please fix a stupid mistake here, and use this instead:

+      if (!MINI_WINDOW_P (w)
+	  && (scroll_conservatively > SCROLL_LIMIT || NUMBERP (aggressive)))

Re: Avoid recentering when user says so

[David Engster]

Eli Zaretskii writes:
> Would people who use these variables please try the patch below, and
> report any findings, both positive and negative?  Please report your
> findings to the bug tracker at 6671@debbugs.gnu.org, so that the
> details get filed there.
>
> Given positive feedback, I will commit this to the trunk.

Oh, please do! Using this patch, scrolling is definitely *much* improved
with my setup. No more annoying recenterings - Thank you!

-David

Re: Avoid recentering when user says so

[Eli Zaretskii]

> From: David Engster <deng@randomsample.de>
> Date: Sun, 27 Mar 2011 09:58:51 +0200
> Cc: 6671@debbugs.gnu.org, emacs-devel@gnu.org
> 
> Eli Zaretskii writes:
> > Would people who use these variables please try the patch below, and
> > report any findings, both positive and negative?  Please report your
> > findings to the bug tracker at 6671@debbugs.gnu.org, so that the
> > details get filed there.
> >
> > Given positive feedback, I will commit this to the trunk.
> 
> Oh, please do! Using this patch, scrolling is definitely *much* improved
> with my setup. No more annoying recenterings - Thank you!

Thanks for the feedback.

Re: Avoid recentering when user says so

[Eli Zaretskii]

> Date: Sat, 26 Mar 2011 20:48:35 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> This is related to bug #6671, read there if you want to know the gory
> details.
> 
> In a nutshell, Emacs currently doesn't honor meticulously enough user
> settings such as scroll-conservatively and scroll-up/down-aggressively.
> Sometimes, especially if redisplay cannot keep up with high-rate
> keyboard input, it falls back on recentering, which these variables
> are supposed to avoid.  The last attempt to solve this introduced a
> bug that is the subject of bug #6671.
> 
> Following the latest discussions in that bug report, I came up with a
> way to solve this problem without incurring unduly slow movement far
> away in the buffer.  The patch below seems to give good results, but
> as I don't use the above settings (I like the default recentering),
> I'm not a good candidate for testing the patch.
> 
> Would people who use these variables please try the patch below, and
> report any findings, both positive and negative?  Please report your
> findings to the bug tracker at 6671@debbugs.gnu.org, so that the
> details get filed there.
> 
> Given positive feedback, I will commit this to the trunk.

Done.  Thanks to all who tried the patch and provided feedback.