Re: Scrolling and follow-mode

[Anders Lindgren <andlind@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 11398 bytes --]

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 <acm@muc.de> 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).
> `
>

[-- Attachment #2: Type: text/html, Size: 14649 bytes --]