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

[Tino Calancha]

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 <address@hidden>
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 address@hidden 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.
+
address@hidden 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:
address@hidden set point at the beginning of the new output,
address@hidden set point at the end of the new output, and
address@hidden 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