bug#24213: 25.1.50; Fishy use of delete-window in register.el

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Andreas Politz]

The register.el commands display a preview of register contents, using
display-buffer in register-preview.  Later, when some register was
chosen by the user, the window showing this preview is closed via
delete-window.

It seems to me, that this call should be replaced by a call to
quit-window, because this function does the right thing: It only deletes
the window if it was previously created and otherwise shows the previous
buffer in that window.

If that analysis can be agreed upon: Grepping through lisp/*, I found 62
uses of delete-window outside window.el.  I assume that most of those
applications are problematic in the same way (e.g. finder.el).

-Andreas

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Leo Liu]

On 2016-08-12 22:22 +0200, Andreas Politz wrote:
> The register.el commands display a preview of register contents, using
> display-buffer in register-preview.  Later, when some register was
> chosen by the user, the window showing this preview is closed via
> delete-window.

The window is created by `display-buffer-below-selected' so we know its
ours and thus delete-window is technically safe here. Use
quit-window/quit-windows-on is ok as well for being defensive.

Martin,

it seems the documentation of display-buffer-below-selected isn't clear
that it only reuses a window if it displays BUFFER already.

Leo

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Andreas Politz]

Leo Liu <sdl.web@gmail.com> writes:

> The window is created by `display-buffer-below-selected' so we know its
> ours [...]

No, the window is created by display-buffer, which is not obligated to
use it's action argument, e.g. if the buffer triggers a match in
display-buffer-alist.  

Maybe the documentation of delte-window should be amended, stating that
it is almost always preferable to use quit-window instead; unless the
window was created by using primitives like split-window.

Thanks,
Andreas

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 > The register.el commands display a preview of register contents, using
 > display-buffer in register-preview.  Later, when some register was
 > chosen by the user, the window showing this preview is closed via
 > delete-window.
 >
 > It seems to me, that this call should be replaced by a call to
 > quit-window, because this function does the right thing: It only deletes
 > the window if it was previously created and otherwise shows the previous
 > buffer in that window.
 >
 > If that analysis can be agreed upon: Grepping through lisp/*, I found 62
 > uses of delete-window outside window.el.  I assume that most of those
 > applications are problematic in the same way (e.g. finder.el).

I agree on that analysis.  At least for every window created or used by
‘display-buffer’ the appropriate action should be ‘quit-window’.  Could
you provide a patch fixing the (about ten) most obvious cases first?

Thanks, martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 > The window is created by `display-buffer-below-selected' so we know its
 > ours and thus delete-window is technically safe here. Use
 > quit-window/quit-windows-on is ok as well for being defensive.
 >
 > Martin,
 >
 > it seems the documentation of display-buffer-below-selected isn't clear
 > that it only reuses a window if it displays BUFFER already.

Indeed.  I tried to correct that on master, please have a look.  Note
that if there is a non-dedicated window below the selected one showing
some other buffer, that window may get used and is strictly spoken not
"ours".  Hopefully, ‘quit-window’ does not delete it.

martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 > Maybe the documentation of delte-window should be amended, stating that
 > it is almost always preferable to use quit-window instead;

‘delete-window’ is primarily a command bound to C-x 0.  It should be
used only sparingly in Elisp code.  Nevertheless, I think that any
advice to use ‘quit-window’ instead should go to the ‘display-buffer’
group functions because ‘quit-window’ (now) inherently belongs there.

 > unless the
 > window was created by using primitives like split-window.

‘split-window’ should not be used in Elisp code either unless it's done
on a separate, special purpose frame.  Standard settings are usually too
restrictive to make ‘split-window’ behave decently.

martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Andreas Politz]

martin rudalics <rudalics@gmx.at> writes:

> Could you provide a patch fixing the (about ten) most obvious cases
> first?

Yes, will do.  I'm going to write a series of independent patches each
with around 10 substitutions; till it's all done.  Would that work for
you ?

-ap

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Leo Liu]

On 2016-08-13 10:24 +0200, martin rudalics wrote:
> Indeed.  I tried to correct that on master, please have a look.

Thanks, the doc looks clear to me.

Leo

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Leo Liu]

On 2016-08-13 08:43 +0200, Andreas Politz wrote:
> No, the window is created by display-buffer, which is not obligated to
> use it's action argument, e.g. if the buffer triggers a match in
> display-buffer-alist.

OK, sounds like quit-window is better.

Leo

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 > I'm going to write a series of independent patches each
 > with around 10 substitutions; till it's all done.  Would that work for
 > you ?

Yes.

Thanks for working on this, martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[npostavs]

martin rudalics <rudalics@gmx.at> writes:

>> Maybe the documentation of delte-window should be amended, stating that
>> it is almost always preferable to use quit-window instead;
>
> ‘delete-window’ is primarily a command bound to C-x 0.  It should be
> used only sparingly in Elisp code.  Nevertheless, I think that any
> advice to use ‘quit-window’ instead should go to the ‘display-buffer’
> group functions because ‘quit-window’ (now) inherently belongs there.

Wouldn't it then make sense to add the usual "if you are thinking of
using this function from Lisp code, it's probably a mistake, try
`quit-window' instead" warning to the delete-window's docstring?

(also, doc fixes should go to emacs-25, right?)

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Sat, 13 Aug 2016 08:44:54 -0400
> Cc: 24213@debbugs.gnu.org, Leo Liu <sdl.web@gmail.com>,
> 	Andreas Politz <politza@hochschule-trier.de>
> 
> (also, doc fixes should go to emacs-25, right?)

Yes.

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 >> ‘delete-window’ is primarily a command bound to C-x 0.  It should be
 >> used only sparingly in Elisp code.  Nevertheless, I think that any
 >> advice to use ‘quit-window’ instead should go to the ‘display-buffer’
 >> group functions because ‘quit-window’ (now) inherently belongs there.
 >
 > Wouldn't it then make sense to add the usual "if you are thinking of
 > using this function from Lisp code, it's probably a mistake, try
 > `quit-window' instead" warning to the delete-window's docstring?

Then readers might think that they should replace ‘delete-window’ with
‘quit-window’ indiscriminately.  That would be incorrect because without
an accompanying ‘quit-restore’ parameter, ‘quit-window’ is not very
useful.  (The history of ‘quit-window’ and ‘quit-restore-window’ is a
convoluted one - I never wanted to commingle these but Chong overruled
me, if my memory doesn't fail).

Paired with ‘split-window’, ‘delete-window’ is the correct choice to
undo the former.  So any warning should go to ‘split-window’ instead.
But then readers might be tempted to replace ‘split-window’ calls with
‘display-buffer’ calls indiscriminately and I don't want to think of the
consequences of this.

 > (also, doc fixes should go to emacs-25, right?)

I was tempted to do that.  But please let's postpone further changes in
this area to the time when Andreas has posted his first patch.  I'm glad
that someone has started looking into this and suppose that further doc
fixes will be necessary.

martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Andreas Politz]

I finally got around to this.  The changes in the following diff seem to
be all clear cut cases, where the window is created by
display-buffer/pop-to-buffer without any special window-handling logic
attached.  Let me know what you think.  

diff --git a/lisp/finder.el b/lisp/finder.el
index da537a5..7f08024 100644
--- a/lisp/finder.el
+++ b/lisp/finder.el
@@ -455,7 +455,7 @@ finder-exit
   "Exit Finder mode.
 Delete the window and kill all Finder-related buffers."
   (interactive)
-  (ignore-errors (delete-window))
+  (quit-window)
   (let ((buf "*Finder*"))
     (and (get-buffer buf) (kill-buffer buf))))
 
diff --git a/lisp/image-dired.el b/lisp/image-dired.el
index 67b023d..36f148b 100644
--- a/lisp/image-dired.el
+++ b/lisp/image-dired.el
@@ -2445,13 +2445,9 @@ image-dired-gallery-generate
       (insert "</html>"))))
 
 (defun image-dired-kill-buffer-and-window ()
-  "Kill the current buffer and, if possible, also the window."
+  "Kill the current buffer and quit it's window."
   (interactive)
-  (let ((buffer (current-buffer)))
-    (condition-case nil
-        (delete-window (selected-window))
-      (error nil))
-    (kill-buffer buffer)))
+  (quit-window t))
 
 (defvar image-dired-widget-list nil
   "List to keep track of meta data in edit buffer.")
diff --git a/lisp/progmodes/idlwave.el b/lisp/progmodes/idlwave.el
index 9cb2ca7..2215e52 100644
--- a/lisp/progmodes/idlwave.el
+++ b/lisp/progmodes/idlwave.el
@@ -8272,7 +8272,7 @@ idlwave-quit-help
       (select-window olh-window)
       (idlwave-help-quit))
     (when (window-live-p ri-window)
-      (delete-window ri-window))))
+      (quit-window nil ri-window))))
 
 (defun idlwave-display-calling-sequence (name type class
 					      &optional initial-class)
diff --git a/lisp/progmodes/verilog-mode.el b/lisp/progmodes/verilog-mode.el
index fd2e96a..a96ddae 100644
--- a/lisp/progmodes/verilog-mode.el
+++ b/lisp/progmodes/verilog-mode.el
@@ -7486,8 +7486,7 @@ verilog-complete-word
 	       (display-completion-list allcomp))
 	     ;; Wait for a key press. Then delete *Completion*  window
 	     (momentary-string-display "" (point))
-	     (delete-window (get-buffer-window (get-buffer "*Completions*")))
-	     )))))
+	     (quit-window nil (get-buffer-window "*Completions*")))))))
 
 (defun verilog-show-completions ()
   "Show all possible completions at current point."
@@ -7506,7 +7505,7 @@ verilog-show-completions
       (display-completion-list allcomp))
     ;; Wait for a key press. Then delete *Completion*  window
     (momentary-string-display "" (point))
-    (delete-window (get-buffer-window (get-buffer "*Completions*")))))
+    (quit-window nil (get-buffer-window "*Completions*"))))
 
 
 (defun verilog-get-default-symbol ()
diff --git a/lisp/startup.el b/lisp/startup.el
index fcdc376..0b21f4f 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1765,9 +1765,7 @@ fancy-startup-tail
 		  (customize-set-variable 'inhibit-startup-screen t)
 		  (customize-mark-to-save 'inhibit-startup-screen)
 		  (custom-save-all))
-		(let ((w (get-buffer-window "*GNU Emacs*")))
-		  (and w (not (one-window-p)) (delete-window w)))
-		(kill-buffer "*GNU Emacs*")))
+		(quit-windows-on "*GNU Emacs*" t)))
      "  ")
     (when (or user-init-file custom-file)
       (let ((checked (create-image "checked.xpm"
diff --git a/lisp/strokes.el b/lisp/strokes.el
index 5a2020d..215f24b 100644
--- a/lisp/strokes.el
+++ b/lisp/strokes.el
@@ -1216,9 +1216,7 @@ strokes-xpm-for-stroke
 
 ;;(defun strokes-edit-quit ()
 ;;  (interactive)
-;;  (or (one-window-p t 0)
-;;      (delete-window))
-;;  (kill-buffer "*Strokes List*"))
+;;  (quit-windows-on "*Strokes List*" t))
 
 ;;(define-derived-mode edit-strokes-mode list-mode
 ;;  "Edit-Strokes"

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 > I finally got around to this.  The changes in the following diff seem to
 > be all clear cut cases, where the window is created by
 > display-buffer/pop-to-buffer without any special window-handling logic
 > attached.  Let me know what you think.

Thank you.


 >     "Exit Finder mode.
 >   Delete the window and kill all Finder-related buffers."

Maybe we should say that we "Quit" the window and "kill the Finder
buffer" here.

 >     (interactive)
 > -  (ignore-errors (delete-window))
 > +  (quit-window)
 >     (let ((buf "*Finder*"))
 >       (and (get-buffer buf) (kill-buffer buf))))

How about using (quit-window t) right away as in the next case?

 > diff --git a/lisp/image-dired.el b/lisp/image-dired.el
 > index 67b023d..36f148b 100644
 > --- a/lisp/image-dired.el
 > +++ b/lisp/image-dired.el
 > @@ -2445,13 +2445,9 @@ image-dired-gallery-generate
 >         (insert "</html>"))))
 >
 >   (defun image-dired-kill-buffer-and-window ()
 > -  "Kill the current buffer and, if possible, also the window."
 > +  "Kill the current buffer and quit it's window."

"its" not "it's".


The remainder looks OK.  If no-one objects, please provide a ChangeLog
entry and I'll install the changes.

Thanks again, martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Andreas Politz]

martin rudalics <rudalics@gmx.at> writes:

>>     "Exit Finder mode.
>>   Delete the window and kill all Finder-related buffers."
>
> Maybe we should say that we "Quit" the window [...]

Yes, I missed that.

>>     (interactive)
>> -  (ignore-errors (delete-window))
>> +  (quit-window)
>>     (let ((buf "*Finder*"))
>>       (and (get-buffer buf) (kill-buffer buf))))
>
> How about using (quit-window t) right away as in the next case?

The resulting function would be quite murderous and try to kill any
buffer regardless of Finder.  I tried to modify the functions semantics
as little as possible.  Who knows in what way people are using them,
right ?


>> diff --git a/lisp/image-dired.el b/lisp/image-dired.el
>> index 67b023d..36f148b 100644
>> --- a/lisp/image-dired.el
>> +++ b/lisp/image-dired.el
>> @@ -2445,13 +2445,9 @@ image-dired-gallery-generate
>>         (insert "</html>"))))
>>
>>   (defun image-dired-kill-buffer-and-window ()
>> -  "Kill the current buffer and, if possible, also the window."
>> +  "Kill the current buffer and quit it's window."
>
> "its" not "it's".

I think I'm using this false possessive apostrophe for years, without
realizing its ambiguity.  Thank you.

More to come.

-ap

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[martin rudalics]

 >> How about using (quit-window t) right away as in the next case?
 >
 > The resulting function would be quite murderous and try to kill any
 > buffer regardless of Finder.  I tried to modify the functions semantics
 > as little as possible.  Who knows in what way people are using them,
 > right ?

Right.  Let's stick to your version.

 > More to come.

Thanks, martin

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Lars Ingebrigtsen]

Andreas Politz <politza@hochschule-trier.de> writes:

> I finally got around to this.  The changes in the following diff seem to
> be all clear cut cases, where the window is created by
> display-buffer/pop-to-buffer without any special window-handling logic
> attached.  Let me know what you think.  

Apparently the patch was not applied at the time, and many of the
instances have later been fixed.

I've respun the patch for Emacs 27, and the remaining fixups (replacing
delete-window with quit-window look logical to me.

Does anybody have any comments?

diff --git a/lisp/progmodes/idlwave.el b/lisp/progmodes/idlwave.el
index 1b4b55c94f..3535a7b4aa 100644
--- a/lisp/progmodes/idlwave.el
+++ b/lisp/progmodes/idlwave.el
@@ -8271,7 +8271,7 @@ idlwave-quit-help
       (select-window olh-window)
       (idlwave-help-quit))
     (when (window-live-p ri-window)
-      (delete-window ri-window))))
+      (quit-window nil ri-window))))
 
 (defun idlwave-display-calling-sequence (name type class
 					      &optional initial-class)
diff --git a/lisp/progmodes/verilog-mode.el b/lisp/progmodes/verilog-mode.el
index baeaf8b95d..5bfa5f837a 100644
--- a/lisp/progmodes/verilog-mode.el
+++ b/lisp/progmodes/verilog-mode.el
@@ -7598,8 +7598,7 @@ verilog-complete-word
 	       (display-completion-list allcomp))
 	     ;; Wait for a key press. Then delete *Completion*  window
 	     (momentary-string-display "" (point))
-	     (delete-window (get-buffer-window (get-buffer "*Completions*")))
-	     )))))
+	     (quit-window nil (get-buffer-window "*Completions*")))))))
 
 (defun verilog-show-completions ()
   "Show all possible completions at current point."
@@ -7611,7 +7610,8 @@ verilog-show-completions
     (display-completion-list (nth 2 (verilog-completion-at-point))))
   ;; Wait for a key press. Then delete *Completion*  window
   (momentary-string-display "" (point))
-  (delete-window (get-buffer-window (get-buffer "*Completions*"))))
+  (quit-window nil (get-buffer-window "*Completions*")))
+
 
 (defun verilog-get-default-symbol ()
   "Return symbol around current point as a string."
diff --git a/lisp/startup.el b/lisp/startup.el
index 52d4dbb05c..7eb0180d41 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1853,9 +1853,7 @@ fancy-startup-tail
 		  (customize-set-variable 'inhibit-startup-screen t)
 		  (customize-mark-to-save 'inhibit-startup-screen)
 		  (custom-save-all))
-		(let ((w (get-buffer-window "*GNU Emacs*")))
-		  (and w (not (one-window-p)) (delete-window w)))
-		(kill-buffer "*GNU Emacs*")))
+		(quit-windows-on "*GNU Emacs*" t)))
      "  ")
     (when (or user-init-file custom-file)
       (let ((checked (create-image "checked.xpm"
diff --git a/lisp/strokes.el b/lisp/strokes.el
index 6edf58c7b6..2c88b1eefd 100644
--- a/lisp/strokes.el
+++ b/lisp/strokes.el
@@ -1216,9 +1216,7 @@ strokes-xpm-for-stroke
 
 ;;(defun strokes-edit-quit ()
 ;;  (interactive)
-;;  (or (one-window-p t 0)
-;;      (delete-window))
-;;  (kill-buffer "*Strokes List*"))
+;;  (quit-windows-on "*Strokes List*" t))
 
 ;;(define-derived-mode edit-strokes-mode list-mode
 ;;  "Edit-Strokes"


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#24213: 25.1.50; Fishy use of delete-window in register.el

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Apparently the patch was not applied at the time, and many of the
> instances have later been fixed.
>
> I've respun the patch for Emacs 27, and the remaining fixups (replacing
> delete-window with quit-window look logical to me.
>
> Does anybody have any comments?

There were no comments in two weeks, so I've now applied the patch, and
I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no