Re: [Emacs-diffs] master c711991: Allow not erase output buffer in shell commands

[Tino Calancha <tino.calancha@gmail.com>]

Hi Stefan,

thank you very much for your comments.
I have addressed all of them.  See the patch below.

On Wed, 22 Aug 2016, Stefan Monnier wrote:

>misc.texi *is* the documentation, so it makes no sense for it to say "see the
>documentation of the variable for details".
>etc/NEWS on the other hand is *not* where we put documentation.  So the
>above shouldn't be a paragraph but a just a line announcing the new var.
Fixed.  Thanks.

>> +(defcustom shell-command-not-erase-buffer nil

>Usually we use "dont" or "inhibit" (please check to see which is more
>standard/common within the core Elisp code, or better yet: which is
>better) rather than just "not".
"inhibit" appears factor ~2 times than "dont".  I have chosen "dont"
which is shorter and quit clear.

>> +  "If non-nil, output buffer is not erased between shell commands.

>You might as well say "before" rather than "between", so it's more precise.
Fixed.  Thanks.

>> +Also, a non-nil value set the point in the output buffer
>> +once the command complete.

>Why use a single var for those two meanings?
>The `end-last-out' choice makes a lot of sense also when combined with
>"do erase", yet your system prevents me from using both at the same time.
Added a new option 'shell-command-set-buffer-point' which controls the point
position.

>> +The value `beg-last-out' set point at the beginning of the output,

>Try to be more explicit about which output we're talking about.
Eg. here I'd probably say "beginning of the new output".

>> +`end-last-out' set point at the end of the buffer,

>And here "end of the new output".
Fixd.  Thanks.

>> `save-point' +restore the buffer position before the command."

>Makes it sound like we restore the buffer position and then we run the
>command, rather than after the command we restore the position it
>had earlier.
I reduced the sentence to just say: and `save-point' restore the point.
From the context should be clear that the point is set to the value
before the command started.

>> +(defvar shell-command-saved-pos nil

>This is an internal variable.  So its name should start with
>"shell-command--" (notice the double dash).
Fixed.  Thanks.

>> +It is an alist (BUFFER . POS), where BUFFER is the output
>> +buffer, and POS is the point position in BUFFER once the
>> command finish.

>Why use an alist rather than set the variable buffer-locally?
Not coffee enough when i wrote that part.
Changed to a permanent buffer local variable.  Thanks.

>> +This variable is used when `shell-command-not-erase-buffer' is non-nil.")

>Variable should generally say what they are expected to hold, rather
>than who uses them.
Dropped the excess of literature.  Thank you.

>> +(defun shell-command--save-pos-or-erase ()
                        ^^
>                     great!
I have my moments... sometimes; don't get used.

>> +  (let ((sym shell-command-not-erase-buffer)

>`sym' is an odd name since we don't care about the fact that it's
>a symbol (we're not going to call things like symbol-name, ...).
Not uing `sym' anymore.  Thanks.

>> +        pos)
>> +    (setq buffer-read-only nil)

>you can move this (setq buffer-read-only nil) before the previous `let',
>so that you can then fold the `setq' of `pos' into the `let'.
Good idea,  fixed.  Thank you.

>> +    ;; Setting buffer-read-only to nil doesn't suffice
>> +    ;; if some text has a non-nil read-only property,
>> +    ;; which comint sometimes adds for prompts.
>> +    (setq pos
>> +          (cond ((eq sym 'save-point) (point))
>> +                ((eq sym 'beg-last-out) (point-max))
>> +                ((not sym)
>> +                 (let ((inhibit-read-only t))
>> +                   (erase-buffer) nil))))

>Here you can use

>       (pcase shell-command-not-erase-buffer
>         ('save-point ...)
>         ('beg-last-out ...)
>         ... )

>instead.
Implemented.  Thank you.

>> +      (when (buffer-live-p buf)
>> +        (let ((win   (car (get-buffer-window-list buf)))

>Why use get-buffer-window-list rather than get-buffer-window?
>Why use a nil value for `all-frames'?
I was doing something wrong.  Fixed.  Thank you.

>> +              (pmax  (with-current-buffer buf (point-max))))
>> +          (unless (and pos (memq sym '(save-point beg-last-out)))
>> +            (setq pos pmax))

>Why not compute pmax here on the fly, rather than pre-compute it before
>we know if we'll need it?
Right.  Fixed.  Thanks.

>> +          ;; Set point in the window displaying buf, if any; otherwise
>> +          ;; display buf temporary in selected frame and set the point.
>> +          (if win
>> +              (set-window-point win pos)
>> +            (save-window-excursion
>> +              (let ((win (display-buffer
>> +                          buf
>> +                          '(nil (inhibit-switch-frame . t)))))
>> +                (set-window-point win pos)))))))))

>You *cannot* undo a `display-buffer' after the fact (the display-buffer
>call can have very visible side-effects in itself.  For example, in my
>setup, it creates a new frame and waits for me to manually place this
>frame on the screen.  No amount of save-window-excursion will make me
>forget that I had to place the frame manually, and worse yet: it might
>even fail to get rid of that new frame).
>
>If the buffer is not displayed, then there's simply no set-window-point
>to perform: use `goto-char' instead.
Ok, fixed as you suggest.
I guess, in cases where the buffer is not displayed, (goto-char pos)
might not always work as `set-window-point': a posterior
display of the buffer maybe doesn't show point at pos.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From f87db51ef46d4ad03cdc612d4fded5ecef39df41 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Tue, 20 Sep 2016 23:18:07 +0900
Subject: [PATCH] Split shell-command-dont-erase-buffer in 2 options

One controls if the output buffer should be erased; the
other controls the position of the point after
the command completes.
Suggested by Stefan Monnier in:
https://lists.gnu.org/archive/html/emacs-devel/2016-08/msg00463.html
* lisp/simple.el (shell-command-set-buffer-point): New option;
control position of point in the output buffer after command completes.
(shell-command-dont-erase-buffer): Fix doc string.
(shell-command--saved-pos): Rename from shell-command-saved-pos.
Make permanent buffer local.
(shell-command--save-pos-or-erase): Use shell-command-set-buffer-point.
(shell-command--set-point-after-cmd): Idem.
Do not force a display of the output buffer.
(shell-command-on-region): Use shell-command-set-buffer-point.
* doc/emacs/misc.texi (shell-command-dont-erase-buffer)
(shell-command-set-buffer-point): Update documentation.
;* etc/NEWS (Changes in Emacs 25.2):
;Add entry for shell-command-set-buffer-point.
;Update entry for shell-command-dont-erase-buffer.
---
  doc/emacs/misc.texi | 18 +++++++---
  etc/NEWS            | 12 +++----
  lisp/simple.el      | 98 +++++++++++++++++++++++++++--------------------------
  3 files changed, 69 insertions(+), 59 deletions(-)

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 502ccad..54e2a3e 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -772,12 +772,22 @@ Single Shell
  inserted into a buffer of that name.

  @vindex shell-command-dont-erase-buffer
-  By default, the output buffer is erased between shell commands.
+  By default, the output buffer is erased before shell commands.
  If you change the value of the variable
  @code{shell-command-dont-erase-buffer} to a non-@code{nil} value,
-the output buffer is not erased.  This variable also controls where to
-set the point in the output buffer after the command completes; see the
-documentation of the variable for details.
+the output buffer is not erased.
+
+@vindex shell-command-set-buffer-point
+  The variable @code{shell-command-set-buffer-point} controls where
+to set the point in the output buffer after the command completes:
+@code{beg-last-out} set point at the beginning of the new output,
+@code{end-last-out} set point at the end of the new output, and
+@code{save-point} restores the point to its position before the
+command started.
+
+  When @code{shell-command-not-erase-buffer} is @code{nil}, the default
+value, the position of the point is unspecified, i.e., an implementation
+detail.

  @node Interactive Shell
  @subsection Interactive Subshell
diff --git a/etc/NEWS b/etc/NEWS
index 9b992d0..6ba7581 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -68,13 +68,11 @@ inferior shell with the buffer region as input.

  +++
  ** The new user option 'shell-command-dont-erase-buffer' controls
-if the output buffer is erased between shell commands; if non-nil,
-the output buffer is not erased; this variable also controls where
-to set the point in the output buffer: beginning of the output,
-end of the buffer or save the point.
-When 'shell-command-dont-erase-buffer' is nil, the default value,
-the behaviour of 'shell-command', 'shell-command-on-region' and
-'async-shell-command' is as usual.
+if the output buffer is erased before shell commands.
+
++++
+** The new user option 'shell-command-set-buffer-point' controls
+where to set point in the output buffer of a shell command.

  +++
  ** The new user option 'mouse-select-region-move-to-beginning'
diff --git a/lisp/simple.el b/lisp/simple.el
index 7e68baa..7b5e556 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -38,25 +38,31 @@ compilation-current-error
  (defvar compilation-context-lines)

  (defcustom shell-command-dont-erase-buffer nil
-  "If non-nil, output buffer is not erased between shell commands.
-Also, a non-nil value set the point in the output buffer
-once the command complete.
-The value `beg-last-out' set point at the beginning of the output,
-`end-last-out' set point at the end of the buffer, `save-point'
-restore the buffer position before the command."
+  "If non-nil, output buffer is not erased before shell commands."
    :type '(choice
            (const :tag "Erase buffer" nil)
+          (const :tag "Don't erase buffer" t))
+  :group 'shell
+  :version "25.2")
+
+(defcustom shell-command-set-buffer-point nil
+  "If non-nil, set point in the output buffer once the command complete.
+The value `beg-last-out' set point at the beginning of the new output,
+`end-last-out' set point at the end of the new output, and `save-point'
+restore the point.
+Note that `save-point' only has sense if `shell-command-dont-erase-buffer'
+is non-nil."
+  :type '(choice
+          (const :tag "None" nil)
            (const :tag "Set point to beginning of last output" beg-last-out)
            (const :tag "Set point to end of last output" end-last-out)
-          (const :tag "Save point" save-point))
+          (const :tag "Restore point" save-point))
    :group 'shell
    :version "25.2")

-(defvar shell-command-saved-pos nil
-  "Point position in the output buffer after command complete.
-It is an alist (BUFFER . POS), where BUFFER is the output
-buffer, and POS is the point position in BUFFER once the command finish.
-This variable is used when `shell-command-dont-erase-buffer' is non-nil.")
+(defvar-local shell-command--saved-pos nil
+  "Point position in the output buffer after command complete.")
+(put 'shell-command--saved-pos 'permanent-local t)

  (defcustom idle-update-delay 0.5
    "Idle time delay before updating various things on the screen.
@@ -3234,49 +3240,44 @@ async-shell-command-buffer
  (defun shell-command--save-pos-or-erase ()
    "Store a buffer position or erase the buffer.
  See `shell-command-dont-erase-buffer'."
-  (let ((sym shell-command-dont-erase-buffer)
-        pos)
-    (setq buffer-read-only nil)
-    ;; Setting buffer-read-only to nil doesn't suffice
-    ;; if some text has a non-nil read-only property,
-    ;; which comint sometimes adds for prompts.
-    (setq pos
-          (cond ((eq sym 'save-point) (point))
-                ((eq sym 'beg-last-out) (point-max))
-                ((not sym)
-                 (let ((inhibit-read-only t))
-                   (erase-buffer) nil))))
+  (setq buffer-read-only nil)
+  ;; Setting buffer-read-only to nil doesn't suffice
+  ;; if some text has a non-nil read-only property,
+  ;; which comint sometimes adds for prompts.
+  (let ((pos (pcase shell-command-set-buffer-point
+               ('save-point (and shell-command-dont-erase-buffer (point)))
+               ('beg-last-out (if shell-command-dont-erase-buffer
+                                  (point-max)
+                                (point-min)))
+               ((pred null) nil))))
+    (unless shell-command-dont-erase-buffer
+      (let ((inhibit-read-only t))
+        (erase-buffer)))
+    (goto-char (point-max))
      (when pos
-      (goto-char (point-max))
-      (push (cons (current-buffer) pos)
-            shell-command-saved-pos))))
+      (setq shell-command--saved-pos pos))))

  (defun shell-command--set-point-after-cmd (&optional buffer)
    "Set point in BUFFER after command complete.
  BUFFER is the output buffer of the command; if nil, then defaults
  to the current BUFFER.
-Set point to the `cdr' of the element in `shell-command-saved-pos'
-whose `car' is BUFFER."
-  (when shell-command-dont-erase-buffer
-    (let* ((sym  shell-command-dont-erase-buffer)
-           (buf  (or buffer (current-buffer)))
-           (pos  (alist-get buf shell-command-saved-pos)))
-      (setq shell-command-saved-pos
-            (assq-delete-all buf shell-command-saved-pos))
+Set point to `shell-command--saved-pos'"
+  (when (and shell-command-set-buffer-point
+             ;; 'save-point is undefined when we erase the buffer.
+             (not (and (eq shell-command-set-buffer-point 'save-point)
+                       (not shell-command-dont-erase-buffer))))
+    (let ((buf (or buffer (current-buffer))))
        (when (buffer-live-p buf)
-        (let ((win   (car (get-buffer-window-list buf)))
-              (pmax  (with-current-buffer buf (point-max))))
-          (unless (and pos (memq sym '(save-point beg-last-out)))
-            (setq pos pmax))
-          ;; Set point in the window displaying buf, if any; otherwise
-          ;; display buf temporary in selected frame and set the point.
+        (set-buffer buf)
+        (let ((win (get-buffer-window nil t))
+              (pos (or shell-command--saved-pos (point-max))))
+          ;; Set point in the window displaying buf, if any.
+          ;; If buf is not displayed, use `goto-char', although this
+          ;; might not set point permanently.
            (if win
                (set-window-point win pos)
-            (save-window-excursion
-              (let ((win (display-buffer
-                          buf
-                          '(nil (inhibit-switch-frame . t)))))
-                (set-window-point win pos)))))))))
+            (goto-char pos))
+          (setq shell-command--saved-pos nil))))))

  (defun async-shell-command (command &optional output-buffer error-buffer)
    "Execute string COMMAND asynchronously in background.
@@ -3562,7 +3563,7 @@ display-message-or-buffer

  ;; We have a sentinel to prevent insertion of a termination message
  ;; in the buffer itself, and to set the point in the buffer when
-;; `shell-command-dont-erase-buffer' is non-nil.
+;; `shell-command-set-buffer-point' is non-nil.
  (defun shell-command-sentinel (process signal)
    (when (memq (process-status process) '(exit signal))
      (shell-command--set-point-after-cmd (process-buffer process))
@@ -3683,7 +3684,8 @@ shell-command-on-region
                         (or output-buffer "*Shell Command Output*"))))
            (unwind-protect
                (if (and (eq buffer (current-buffer))
-                       (or (not shell-command-dont-erase-buffer)
+                       (or (and (not shell-command-dont-erase-buffer)
+                                (not shell-command-set-buffer-point))
                             (and (not (eq buffer (get-buffer "*Shell Command Output*")))
                                  (not (region-active-p)))))
                    ;; If the input is the same buffer as the output,
-- 
2.9.3

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

In GNU Emacs 25.1.50.1
Repository revision: 83fbb3a6dd75e01a768cb6b3348b7c947711ee46