Re: Scrolling and follow-mode

[Anders Lindgren]

Hi Alan and Emacs!

Scrolling a few lines at a time was obviously something that I do very seldom -- it appears that I missed that case when I wrote follow-mode, and I have never seen this problem for the (almost) twenty years I've been using it. Thanks for finding and fixing the problem!

When it comes to the patch, it looks well written. I have only two comments:

* The documentation says that `follow-fixed-window` is the window that must not be scrolled. However, in the code, it's only read once, as the condition to a `when`, so the actual value doesn't matter as long as it's non-nil. Wouldn't it better do document is a boolean?

* The code at (your) lines --- 1201,1222 ---- could be cleaner. It contains a `let*` that sets two variables, `dest` and `windows` and set a bunch others to a default nil value. The others are later set using a `setq`. I would suggest placing your code before the first assignment of `dest` and simply move the point, that way `dest` will get the value of the new point. Then use a new `let*` to set the others. Like the following (untested) code:

(let ((windows (follow-all-followers win)))

  (when follow-fixed-window

    (follow-debug-message "fixed")

    (follow-redisplay windows win)

    (goto-char (follow-get-scrolled-point (point) windows))

    (setq follow-fixed-window nil))

  (let* ((dest (point))

         (win-start-end (progn

                          (follow-update-window-start (car windows))

                          (follow-windows-start-end windows)))

         (aligned (follow-windows-aligned-p win-start-end))

         (visible (follow-pos-visible dest win win-start-end))

         selected-window-up-to-date)

    (unless (and aligned visible)

      (setq follow-windows-start-end-cache nil))

Again, thanks!

Sincerely,

    Anders Lindgren

On Mon, Apr 21, 2014 at 6:17 PM, Alan Mackenzie <address@hidden> wrote:

Hi, Anders, hi, Emacs.

With follow-mode enabled, the functions follow-scroll-up and
follow-scroll-down don't work very well when given a numerical argument.

In follow-scroll-up, the flag follow-internal-force-redisplay gets set,
which causes the post-command-hook function (a) to bypass the code which
selects the window to put point in; (b) to force redisplay to rescroll
the current window.  Both of these are undesirable when point is near a
boundary of one window, and should pass into the adjacent window on the
scrolling.

I have fixed this by causing the two scrolling functions to set a flag
which causes follow-adjust-window to align its windows as its first
action, then allowing it to select the appropriate window for point with
the existing code.  There are one or two minor changes too.

Comments on this fix are welcome.  Here is the patch (based on the
recent trunk revision 116992.




*** emacs/emacs.bzr/trunk/lisp/follow.el        2014-02-15 19:53:57.000000000 +0000
--- follow.el   2014-04-21 16:15:28.000000000 +0000
***************
*** 347,352 ****
--- 347,355 ----
  (defvar follow-windows-start-end-cache nil
    "Cache used by `follow-window-start-end'.")

+ (defvar follow-fixed-window nil
+   "When non-nil, the window which must not be scrolled.
+ This is typically set by explicit scrolling commands.")
  ;;; Debug messages

  ;; This inline function must be as small as possible!
***************
*** 439,444 ****
--- 442,496 ----

  ;;; Scroll

+ (defun follow-get-scrolled-point (dest windows)
+   "Calculate the correct value for point after a scrolling operation.
+
+ DEST is our default position, typically where point was before the scroll.
+ If `scroll-preserve-screen-position' is non-nil and active, DEST will be
+ in the same screen position as before the scroll.  WINDOWS is the list of
+ windows in the follow chain.
+
+ This function attempts to duplicate the point placing from
+ `window_scroll_line_based' in the Emacs core source window.c.
+
+ Return the new position."
+   (if (and scroll-preserve-screen-position
+          (get this-command 'scroll-command))
+       dest
+     (let ((dest-column
+          (save-excursion
+            (goto-char dest)
+            (- (current-column)
+               (progn (vertical-motion 0) (current-column)))))
+         (limit0
+          (with-selected-window (car windows)
+            (save-excursion
+              (goto-char (window-start))
+              (vertical-motion 0)
+              (point))))
+         (limitn
+          (with-selected-window (car (reverse windows))
+            (save-excursion
+              (goto-char (window-end nil t))
+              (if (pos-visible-in-window-p)
+                  (point)              ; i.e. (point-max)
+                (1- (point)))))))
+       (follow-debug-message "dest: %s; dest-column: %s; limitn: %s" dest dest-column limitn)
+       (cond
+        ((< dest limit0)
+       (with-selected-window (car windows)
+         (save-excursion
+           (goto-char limit0)
+           (vertical-motion (cons dest-column 0))
+           (point))))
+        ((> dest limitn)
+       (with-selected-window (car (reverse windows))
+         (save-excursion
+           (goto-char limitn)
+           (vertical-motion (cons dest-column 0))
+           (point))))
+        (t dest)))))
+
  ;; `scroll-up' and `-down', but for windows in Follow mode.
  ;;
  ;; Almost like the real thing, except when the cursor ends up outside
***************
*** 454,459 ****
--- 506,512 ----
  ;; position...  (This would also be corrected if we would have had a
  ;; good redisplay abstraction.)

+ ;;;###autoload
  (defun follow-scroll-up (&optional arg)
    "Scroll text in a Follow mode window chain up.

***************
*** 467,475 ****
    (interactive "P")
    (cond ((not follow-mode)
         (scroll-up arg))
        (arg
!        (save-excursion (scroll-up arg))
!        (setq follow-internal-force-redisplay t))
        (t
         (let* ((windows (follow-all-followers))
                (end (window-end (car (reverse windows)))))
--- 520,534 ----
    (interactive "P")
    (cond ((not follow-mode)
         (scroll-up arg))
+       ((eq arg '-)
+        (follow-scroll-down))
        (arg
!        (let ((opoint (point)))
!          (scroll-up arg)
!          (unless (and scroll-preserve-screen-position
!                       (get this-command 'scroll-command))
!            (goto-char opoint))
!          (setq follow-fixed-window (selected-window))))
        (t
         (let* ((windows (follow-all-followers))
                (end (window-end (car (reverse windows)))))
***************
*** 481,488 ****
                 (goto-char end))
             (vertical-motion (- next-screen-context-lines))
             (set-window-start (car windows) (point)))))))

!
  (defun follow-scroll-down (&optional arg)
    "Scroll text in a Follow mode window chain down.

--- 540,548 ----
                 (goto-char end))
             (vertical-motion (- next-screen-context-lines))
             (set-window-start (car windows) (point)))))))
+ (put 'follow-scroll-up 'scroll-command t)

! ;;;###autoload
  (defun follow-scroll-down (&optional arg)
    "Scroll text in a Follow mode window chain down.

***************
*** 492,503 ****
  If called with an argument, scroll ARG lines down.
  Negative ARG means scroll upward.

! Works like `scroll-up' when not in Follow mode."
    (interactive "P")
    (cond ((not follow-mode)
!        (scroll-up arg))
        (arg
!        (save-excursion (scroll-down arg)))
        (t
         (let* ((windows (follow-all-followers))
                (win (car (reverse windows)))
--- 552,571 ----
  If called with an argument, scroll ARG lines down.
  Negative ARG means scroll upward.

! Works like `scroll-down' when not in Follow mode."
    (interactive "P")
    (cond ((not follow-mode)
!        (scroll-down arg))
!       ((eq arg '-)
!        (follow-scroll-up))
        (arg
!        (let ((opoint (point)))
!          (scroll-down arg)
!          (unless (and scroll-preserve-screen-position
!                       (get this-command 'scroll-command))
!            (goto-char opoint))
!          (setq follow-fixed-window (selected-window)))
!        )
        (t
         (let* ((windows (follow-all-followers))
                (win (car (reverse windows)))
***************
*** 513,518 ****
--- 581,587 ----
             (goto-char start)
             (vertical-motion (- next-screen-context-lines 1))
             (setq follow-internal-force-redisplay t))))))
+ (put 'follow-scroll-down 'scroll-command t)

  (declare-function comint-adjust-point "comint" (window))
  (defvar comint-scroll-show-maximum-output)
***************
*** 1132,1143 ****
               (not (window-minibuffer-p win)))
      (let* ((dest (point))
             (windows (follow-all-followers win))
!            (win-start-end (progn
!                             (follow-update-window-start (car windows))
!                             (follow-windows-start-end windows)))
!            (aligned (follow-windows-aligned-p win-start-end))
!            (visible (follow-pos-visible dest win win-start-end))
             selected-window-up-to-date)
        (unless (and aligned visible)
          (setq follow-windows-start-end-cache nil))

--- 1201,1222 ----
               (not (window-minibuffer-p win)))
      (let* ((dest (point))
             (windows (follow-all-followers win))
!            win-start-end aligned visible
             selected-window-up-to-date)
+
+       ;; If we've explicitly scrolled, align the windows first.
+       (when follow-fixed-window
+       (follow-debug-message "fixed")
+       (follow-redisplay windows win)
+       (setq dest (follow-get-scrolled-point dest windows))
+       (goto-char dest)
+       (setq follow-fixed-window nil))
+       (setq
+        win-start-end (progn
+                      (follow-update-window-start (car windows))
+                      (follow-windows-start-end windows))
+        aligned (follow-windows-aligned-p win-start-end)
+        visible (follow-pos-visible dest win win-start-end))
        (unless (and aligned visible)
          (setq follow-windows-start-end-cache nil))

***************
*** 1184,1190 ****
             ;; It should be optimized for speed.
             ((and visible aligned)
              (follow-debug-message "same"))
!            ;; Pick a position in any window.  If the display is ok,
             ;; this picks the `correct' window.
             ((follow-select-if-visible dest win-start-end)
              (follow-debug-message "visible")
--- 1263,1269 ----
             ;; It should be optimized for speed.
             ((and visible aligned)
              (follow-debug-message "same"))
!          ;; Pick a position in any window.  If the display is ok,
             ;; this picks the `correct' window.
             ((follow-select-if-visible dest win-start-end)
              (follow-debug-message "visible")



--
Alan Mackenzie (Nuremberg, Germany).
`