Re: [PATCH] emacs/show: Only recenter interactively

[Pengji Zhang <me@pengjiz.com>]

Damien Cassou <damien@cassou.me> writes:

> Several notmuch-show commands recenter the window by calling
> `notmuch-show-message-adjust`. This is fine when these commands are
> called interactively but not when used inside non-interactive
> functions. An example of the problem is when `which-func-mode` is
> activated: each time the user moves the point to a different line, the
> window is recentered. This happens because `which-func-mode` calls
> `imenu` which calls `notmuch-show-imenu-prev-index-position-function`
> which calls `notmuch-show-previous-message` which recenters the
> window.
>
> This patch makes sure that commands only invoke
> `notmuch-show-message-adjust` when they have been called
> interactively.

I think this is a reasonable change. Thanks! I am not a maintainer, but
here is my review of your patch.

> ---
>  emacs/notmuch-show.el | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 14e3c698..c08375b8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1535,7 +1535,7 @@ (defun notmuch-show-goto-message (msg-id)
>  		   until (not (notmuch-show-goto-message-next)))
>      (goto-char (point-min))
>      (message "Message-id not found."))
> -  (notmuch-show-message-adjust))
> +  (when (interactive-p) (notmuch-show-message-adjust)))

'notmuch-show-goto-message' is not a command. So it is always called
non-interactively.

>  (defun notmuch-show-apply-state (state)
>    "Apply STATE to the current buffer.
> @@ -2052,7 +2052,7 @@ (defun notmuch-show-rewind ()
>        (when (<= (count-screen-lines (window-start) start-of-message)
>  		next-screen-context-lines)
>  	(goto-char (notmuch-show-message-top))
> -	(notmuch-show-message-adjust))
> +	(when (interactive-p) (notmuch-show-message-adjust)))
>        ;; Move to the top left of the window.
>        (goto-char (window-start)))
>       (t
> @@ -2109,7 +2109,7 @@ (defun notmuch-show-next-message (&optional pop-at-end)
>  thread, navigate to the next thread in the parent search buffer."
>    (interactive "P")
>    (if (notmuch-show-goto-message-next)
> -      (notmuch-show-message-adjust)
> +      (when (interactive-p) (notmuch-show-message-adjust))
>      (if pop-at-end
>  	(notmuch-show-next-thread)
>        (goto-char (point-max)))))
> @@ -2120,7 +2120,7 @@ (defun notmuch-show-previous-message ()
>    (if (= (point) (notmuch-show-message-top))
>        (notmuch-show-goto-message-previous)
>      (notmuch-show-move-to-message-top))
> -  (notmuch-show-message-adjust))
> +  (when (interactive-p) (notmuch-show-message-adjust)))
>  
>  (defun notmuch-show-next-open-message (&optional pop-at-end)
>    "Show the next open message.
> @@ -2134,7 +2134,7 @@ (defun notmuch-show-next-open-message (&optional pop-at-end)
>      (while (and (setq r (notmuch-show-goto-message-next))
>  		(not (notmuch-show-message-visible-p))))
>      (if r
> -	(notmuch-show-message-adjust)
> +	(when (interactive-p) (notmuch-show-message-adjust))
>        (if pop-at-end
>  	  (notmuch-show-next-thread)
>  	(goto-char (point-max))))
> @@ -2147,7 +2147,7 @@ (defun notmuch-show-next-matching-message ()
>      (while (and (setq r (notmuch-show-goto-message-next))
>  		(not (notmuch-show-get-prop :match))))
>      (if r
> -	(notmuch-show-message-adjust)
> +	(when (interactive-p) (notmuch-show-message-adjust))
>        (goto-char (point-max)))))
>  
>  (defun notmuch-show-open-if-matched ()
> @@ -2176,7 +2176,7 @@ (defun notmuch-show-previous-open-message ()
>  		  (notmuch-show-goto-message-previous)
>  		(notmuch-show-move-to-message-top))
>  	      (not (notmuch-show-message-visible-p))))
> -  (notmuch-show-message-adjust))
> +  (when (interactive-p) (notmuch-show-message-adjust)))
>  
>  (defun notmuch-show-view-raw-message ()
>    "View the original source of the current message."

'interactive-p' is a long obsolete function. I think we should at least
use 'called-interactively-p'. Even better, as suggested in the
documentation of 'called-interactively-p', we may add an optional
argument and make use of the interactive form. E.g.,

--8<---------------cut here---------------start------------->8---
-(defun notmuch-show-previous-open-message ()
+(defun notmuch-show-previous-open-message (&optional adjust)
   "Show the previous open message."
-  (interactive)
+  (interactive (list t))
   (while (and (if (= (point) (notmuch-show-message-top))
 		  (notmuch-show-goto-message-previous)
 		(notmuch-show-move-to-message-top))
 	      (not (notmuch-show-message-visible-p))))
-  (notmuch-show-message-adjust))
+  (when adjust (notmuch-show-message-adjust)))
--8<---------------cut here---------------end--------------->8---

Best,
Pengji