From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] master c711991: Allow not erase output buffer in shell commands Date: Mon, 22 Aug 2016 12:41:00 -0400 Message-ID: References: <20160816094403.31386.69310@vcs.savannah.gnu.org> <20160816094404.013EC220155@vcs.savannah.gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1471884200 13323 195.159.176.226 (22 Aug 2016 16:43:20 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 22 Aug 2016 16:43:20 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) Cc: Tino Calancha To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Aug 22 18:43:16 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bbsJf-00037Y-Rf for ged-emacs-devel@m.gmane.org; Mon, 22 Aug 2016 18:43:16 +0200 Original-Received: from localhost ([::1]:42346 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bbsJd-0001n7-8O for ged-emacs-devel@m.gmane.org; Mon, 22 Aug 2016 12:43:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:51111) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bbsHm-0000cj-0t for emacs-devel@gnu.org; Mon, 22 Aug 2016 12:41:19 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bbsHf-0000CS-Vv for emacs-devel@gnu.org; Mon, 22 Aug 2016 12:41:16 -0400 Original-Received: from chene.dit.umontreal.ca ([132.204.246.20]:43400) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bbsHf-0000CI-Qu for emacs-devel@gnu.org; Mon, 22 Aug 2016 12:41:11 -0400 Original-Received: from fmsmemgm.homelinux.net (lechon.iro.umontreal.ca [132.204.27.242]) by chene.dit.umontreal.ca (8.14.7/8.14.1) with ESMTP id u7MGfc6o012103; Mon, 22 Aug 2016 12:41:41 -0400 Original-Received: by fmsmemgm.homelinux.net (Postfix, from userid 20848) id 696A4AE0EA; Mon, 22 Aug 2016 12:41:00 -0400 (EDT) In-Reply-To: <20160816094404.013EC220155@vcs.savannah.gnu.org> (Tino Calancha's message of "Tue, 16 Aug 2016 09:44:03 +0000 (UTC)") X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV5774=0 X-NAI-Spam-Version: 2.3.0.9418 : core <5774> : inlines <5124> : streams <1688302> : uri <2272353> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.20 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:206757 Archived-At: > --- a/doc/emacs/misc.texi > +++ b/doc/emacs/misc.texi > @@ -771,6 +771,14 @@ the output buffer. But if you change the value of the variable > @code{shell-command-default-error-buffer} to a string, error output is > inserted into a buffer of that name. > +@vindex shell-command-not-erase-buffer > + By default, the output buffer is erased between shell commands. > +If you change the value of the variable > +@code{shell-command-not-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. misc.texi *is* the documentation, so it makes no sense for it to say "see the documentation of the variable for details". > --- a/etc/NEWS > +++ b/etc/NEWS > @@ -56,6 +56,16 @@ affected by this, as SGI stopped supporting IRIX in December 2013. > * Changes in Emacs 25.2 > +++ > +** The new user option 'shell-command-not-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-not-erase-buffer' is nil, the default value, > +the behaviour of 'shell-command', 'shell-command-on-region' and > +'async-shell-command' is as usual. 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. > +(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". > + "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. > +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. > +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". > `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. > +(defvar shell-command-saved-pos nil This is an internal variable. So its name should start with "shell-command--" (notice the double dash). > +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? > +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. > +(defun shell-command--save-pos-or-erase () ^^ great! > + (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, ...). > + 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'. > + ;; 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. > + (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'? > + (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? > + ;; 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. Stefan