Re: "Pager" page-up and -down, why not merge?

[Stefan Monnier]

> scroll-up in emacs.  In particular, hitting sequences like [next]  or Ctrl-v
> followed by [previous] or M-v leaves point in the same  place, which is very
> calming.  ;-)

In what way is it different from setting scroll-preserve-screen-position?

> Is there any reason these could not be incorporated into
> emacs to replace scroll-up and scroll-down in these bindings?

The devil is in the details, see comment below.

> (defvar pager-temporary-goal-column 0
>   "Similar to temporary-goal-column but used by the pager.el functions")
> ;(make-variable-buffer-local 'pager-temporary-goal-column)

I suggest we reuse temporary-goal-column instead.

> (defconst pager-keep-column-commands
>   '(pager-row-down pager-row-up row-dn row-up
>                  pager-page-down pager-page-up pg-dn pg-up)
>   "Commands which when called without any other intervening command should
> keep the `pager-temporary-goal-column'")

What are those extra commands?

> (defun pager-scroll-screen (lines)
>   "Scroll screen LINES, but keep the cursors position on screen."
>   (if (not (memq last-command pager-keep-column-commands))
>       (setq pager-temporary-goal-column (current-column)))
>   (save-excursion
>     (goto-char (window-start))
>     (vertical-motion lines)
>     (set-window-start (selected-window) (point)))
>   (vertical-motion lines)
>   (move-to-column pager-temporary-goal-column))

Since this doesn't doesn't use scroll-up or scroll-down, it's very much
unclear whether it's going to behave sufficiently well in all cases.

I'm especially uncomfortable with the use of vertical-motion, which
doesn't actually use the redisplay's code, but some approximation of its
behavior, (so it doesn't pay attention to images and other things like
that, for example).

I'd suggest a different approach: use scroll-up/scroll-down as before, but
wrap them within code that uses compute-motion first to determine the
original screen position, and then vertical-motion afterwards to set
the cursor back to its position (but double-checking that this new
computed position is indeed visible on screen, to avoid pathological
behavior when compute-motion of vertical-motion get it wrong).

This way, you can guarantee that the scrolling behavior is 100%
preserved, and the new code only moves point around within the displayed
part of the buffer presumably to the position it had before.

Instead of compute-motion/vertical-motion, you could try to use
posn-at-point and posn-at-x-y.


        Stefan


PS: Here's a patch to window.c which makes
scroll-preserve-screen-position preserve the column position as well as
the line position, tho it only applies to GUI frames, not to tty frames
(which use another piece of code which I haven't bothered to adjust
accordingly).


=== modified file 'src/window.c'
--- src/window.c        2008-06-03 06:20:49 +0000
+++ src/window.c        2008-06-04 16:10:40 +0000
@@ -229,6 +229,7 @@
 /* Used by the function window_scroll_pixel_based */
 
 static int window_scroll_pixel_based_preserve_y;
+static int window_scroll_pixel_based_preserve_x;
 
 #if 0 /* This isn't used anywhere.  */
 /* Nonzero means we can split a frame even if it is "unsplittable".  */
@@ -5221,10 +5222,12 @@
          start_display (&it, w, start);
          move_it_to (&it, PT, -1, -1, -1, MOVE_TO_POS);
          window_scroll_pixel_based_preserve_y = it.current_y;
+         window_scroll_pixel_based_preserve_x = it.current_x;
        }
     }
   else
-    window_scroll_pixel_based_preserve_y = -1;
+    window_scroll_pixel_based_preserve_y
+      = window_scroll_pixel_based_preserve_x = -1;
 
   /* Move iterator it from start the specified distance forward or
      backward.  The result is the new window start.  */
@@ -5360,10 +5363,11 @@
        {
          /* If we have a header line, take account of it.
             This is necessary because we set it.current_y to 0, above.  */
-         move_it_to (&it, -1, -1,
+         move_it_to (&it, -1,
+                     window_scroll_pixel_based_preserve_x,
                      window_scroll_pixel_based_preserve_y
                      - (WINDOW_WANTS_HEADER_LINE_P (w) ? 1 : 0 ),
-                     -1, MOVE_TO_Y);
+                     -1, MOVE_TO_Y | MOVE_TO_X);
          SET_PT_BOTH (IT_CHARPOS (it), IT_BYTEPOS (it));
        }
       else
@@ -5421,8 +5425,9 @@
          /* It would be wrong to subtract CURRENT_HEADER_LINE_HEIGHT
             here because we called start_display again and did not
             alter it.current_y this time.  */
-         move_it_to (&it, -1, -1, window_scroll_pixel_based_preserve_y, -1,
-                     MOVE_TO_Y);
+         move_it_to (&it, -1, window_scroll_pixel_based_preserve_x,
+                     window_scroll_pixel_based_preserve_y, -1,
+                     MOVE_TO_Y | MOVE_TO_X);
          SET_PT_BOTH (IT_CHARPOS (it), IT_BYTEPOS (it));
        }
       else
@@ -7445,6 +7450,7 @@
   staticpro (&minibuf_selected_window);
 
   window_scroll_pixel_based_preserve_y = -1;
+  window_scroll_pixel_based_preserve_x = -1;
 
   DEFVAR_LISP ("temp-buffer-show-function", &Vtemp_buffer_show_function,
               doc: /* Non-nil means call as function to display a help buffer.