bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

[tlh]

`menu-updating-frame' is pointing to a dead frame, causing a
`frame-live-p' error in `menu-bar-non-minibuffer-p' when I call
`kill-this-buffer'.  I don't how it got out of sync, but it seems to me
that `kill-this-buffer' shouldn't be in menu-bar.el -- or depend on
menu-bar-specific code -- in the first place.



Backtrace:

Debugger entered--Lisp error: (wrong-type-argument frame-live-p #<dead frame *Help* 0x100d1e930>)
  frame-selected-window(#<dead frame *Help* 0x100d1e930>)
  menu-bar-non-minibuffer-window-p()
  kill-this-buffer()
  call-interactively(kill-this-buffer nil nil)





In GNU Emacs 23.1.90.1 (x86_64-apple-darwin10.4.0, NS apple-appkit-1038.32)
 of 2010-07-17 on ridley.local
Windowing system distributor `Apple', version 10.3.1038
configured using `configure  '--with-ns''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: nil
  value of $XMODIFIERS: nil
  locale-coding-system: nil
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

Minor modes in effect:
  erc-services-mode: t
  erc-log-mode: t
  whitespace-mode: t
  eldoc-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  shell-dirtrack-mode: t
  kvdb-mode: t
  recs-mode: t
  recentf-mode: t
  show-paren-mode: t
  workgroups-mode: t
  erc-list-mode: t
  erc-menu-mode: t
  erc-autojoin-mode: t
  erc-ring-mode: t
  erc-networks-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-track-minor-mode: t
  erc-match-mode: t
  erc-netsplit-mode: t
  erc-highlight-nicknames-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  ido-everywhere: t
  auto-image-file-mode: t
  global-auto-revert-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
l l t h i s SPC f u C-a C-H-f C-H-f C-n C-n C-n C-n 
C-n M-f M-f M-< C-s n o n - m i n i b u f f e r - w 
i n d o w - p C-s C-s C-s C-s C-s C-s C-s C-a C-n C-n 
C-n C-n C-n C-n C-n C-p C-e M-b M-f C-c C-e C-H-f C-H-p 
M-b M-b M-b M-b M-b M-b M-b M-b M-b M-b M-b M-f M-f 
M-b M-b M-b M-b C-h C-h t h e SPC v a r SPC ` M-f - 
f r a m e ' M-d M-> <return> I t ' s SPC f u n n y 
! <return> C-H-f C-H-f C-H-p H-P H-P H-P H-P H-P H-P 
H-P H-P H-P H-P H-P H-P H-P M-> C-z C-j C-p H-P H-P 
M-> H-P H-P M-> H-P H-P H-P M-> C-x RET b u g - r e 
p o C-g C-x RET r e p o r t <return> C-g C-H-f C-H-k 
C-z RET C-b C-H-k H-P H-P H-P C-p C-p C-p C-p C-p C-p 
C-p q C-x b <return> C-l C-l C-l C-l C-l C-p C-l C-a 
M-f M-f M-f M-f M-f M-f M-f C-c C-e C-H-f C-x RET r 
e p o r t <return> m e n u - u p d a t i n g - f r 
a m e SPC c a u s C-h C-h C-h C-h M-b M-b M-b o u t 
- o f - s y n c C-h C-h C-h y n c SPC C-e c a u s i 
n g SPC e r r o r s SPC C-h C-h C-h C-h C-h C-h C-h 
C-g C-H-f C-H-k q C-x RET r e p o r <return>

Recent messages:
Mark set [5 times]
Quit [2 times]
menu-bar-non-minibuffer-window-p: Wrong type argument: frame-live-p, #<dead frame *Help* 0x100d1e930>
Debug on Error enabled globally
Entering debugger...
Back to top level.
#<dead frame *Help* 0x100d1e930>
Quit
Entering debugger...
Back to top level.

Load-path shadows:
/Users/luke/emacs/site-lisp/emms/lisp/tq hides /Applications/Emacs.app/Contents/Resources/lisp/emacs-lisp/tq

Features:
(shadow mail-extr emacsbug jka-compr find-func info debug cus-start
cus-load warnings compile erc-services erc-log newcomment multi-isearch
vc-git whitespace tlh-startup ansi-color em-unix em-script em-ls em-hist
em-pred em-glob em-dirs em-basic esh-opt em-alias esh-var esh-io esh-cmd
esh-ext esh-proc esh-arg eldoc esh-groups eshell em-banner em-cmpl
em-term term disp-table ehelp electric em-prompt esh-module esh-mode
esh-util help-mode view tlh-registers tlh-keys tlh-alias tlh-system
tlh-osx tlh-mode ascii-table breadcrumb edit-server goto-last-change
malyon malyon-mode zone tabify undo-tree yaoddmuse url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-util
url-parse url-vars skeleton sgml-mode google-define w3m-load
clojure-mode slime-fontifying-fu slime-package-fu slime-references
slime-scratch slime-presentations slime-fuzzy slime-fancy-inspector
slime-parse slime-editing-commands slime-banner slime-asdf slime-repl
slime apropos hideshow hyperspec browse-url slime-autoloads diary-lib
diary-loaddefs cal-menu calendar cal-loaddefs midnight tramp-imap
imap-hash imap message sendmail ecomplete rfc822 mml mml-sec mm-decode
mm-bodies mm-encode mailcap mail-parse rfc2231 rfc2047 rfc2045 qp
ietf-drums mailabbrev nnheader mm-util mail-prsvr gmm-utils mailheader
canlock sha1 hex-util hashcash mail-utils assoc tramp-gw tramp-fish
tramp-smb tramp-cache tramp-ftp tramp-cmds tramp auth-source gnus-util
netrc shell password-cache tramp-compat trampver tls kvdb org-table org
org-footnote org-src org-list org-faces org-compat org-macs noutline
outline pickel epa-file epa derived epg epg-config uniquify recs-mode
imenu byte-opt browse-kill-ring advice help-fns advice-preload windmove
saveplace recentf tree-widget bbdb-autoloads bbdb timezone paren
color-theme-thunk1 color-theme workgroups tlh-notify tlh-sound tlh-erc
erc-menu erc-join erc-ring comint ring erc-networks erc-pcomplete
time-date pcomplete erc-track erc-match erc-netsplit
erc-highlight-nicknames easy-mmode erc-button erc-fill erc-stamp
wid-edit erc-goodies erc erc-backend erc-compat format-spec thingatpt pp
tlh-emms edmacro kmacro emms-browser sort emms-playlist-sort
emms-last-played emms-cache emms-mode-line-icon emms-mode-line
emms-info-id3v2 emms-info-ogginfo emms-info-mp3info emms-info later-do
emms-playlist-mode emms-player-mplayer emms-player-simple
emms-source-playlist emms-source-file dired emms emms-compat tlh-ido ido
tlh-init delsel regexp-opt image-file autorevert yow cookie1 server
tlh-util cl cl-19 bytecomp byte-compile tooltip ediff-hook vc-hooks
lisp-float-type mwheel ns-win easymenu tool-bar dnd fontset image fringe
lisp-mode register page menu-bar rfn-eshadow timer select scroll-bar
mldrag mouse jit-lock font-lock syntax facemenu font-core frame cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev loaddefs button minibuffer faces cus-face text-properties overlay
md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process ns multi-tty
emacs)

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

[Eli Zaretskii]

> From: tlh <thunkout@gmail.com>
> Date: Sat, 05 Mar 2011 23:28:31 -0600
> Cc: 
> 
> 
> `menu-updating-frame' is pointing to a dead frame, causing a
> `frame-live-p' error in `menu-bar-non-minibuffer-p' when I call
> `kill-this-buffer'.  I don't how it got out of sync, but it seems to me
> that `kill-this-buffer' shouldn't be in menu-bar.el -- or depend on
> menu-bar-specific code -- in the first place.

??? This function is bound to the "Close" item in the "File" menu, so
it is definitely related to the menu bar.

> Backtrace:
> 
> Debugger entered--Lisp error: (wrong-type-argument frame-live-p #<dead frame *Help* 0x100d1e930>)
>   frame-selected-window(#<dead frame *Help* 0x100d1e930>)
>   menu-bar-non-minibuffer-window-p()
>   kill-this-buffer()
>   call-interactively(kill-this-buffer nil nil)

Did you call kill-this-buffer from the menu bar, or with M-x?

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

[martin rudalics]

 > ??? This function is bound to the "Close" item in the "File" menu, so
 > it is definitely related to the menu bar.

Am I right that the time for executing `kill-this-buffer-enabled-p' is
currently always proportional to the number of live buffers?  In that
case I'd rewrite his function as

(defun kill-this-buffer-enabled-p ()
   (or (not (menu-bar-non-minibuffer-window-p))
       (let (found-1)
	(catch 'found-2
	  (dolist (buffer (buffer-list))
	    (unless (string-match-p "^ " (buffer-name buffer))
	      (if (not found-1)
		  (setq found-1 t)
		(throw 'found-2 t))))))))

martin

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[Drew Adams]

> Am I right that the time for executing `kill-this-buffer-enabled-p' is
> currently always proportional to the number of live buffers?

Seems so.

> In that case I'd rewrite [t]his function as
> 
> (defun kill-this-buffer-enabled-p ()
>    (or (not (menu-bar-non-minibuffer-window-p))
>        (let (found-1)
> 	(catch 'found-2
> 	  (dolist (buffer (buffer-list))
> 	    (unless (string-match-p "^ " (buffer-name buffer))
> 	      (if (not found-1)
> 		  (setq found-1 t)
> 		(throw 'found-2 t))))))))

`found-1' is useless here (not used).

(defun kill-this-buffer-enabled-p ()
  (or (not (menu-bar-non-minibuffer-window-p))
      (catch 'found
        (dolist (buffer  (buffer-list))
          (unless (string-match-p "^ " (buffer-name buffer))
            (throw 'found t)))
        nil)))

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[martin rudalics]

 >> (defun kill-this-buffer-enabled-p ()
 >>    (or (not (menu-bar-non-minibuffer-window-p))
 >>        (let (found-1)
 >> 	(catch 'found-2
 >> 	  (dolist (buffer (buffer-list))
 >> 	    (unless (string-match-p "^ " (buffer-name buffer))
 >> 	      (if (not found-1)
 >> 		  (setq found-1 t)
 >> 		(throw 'found-2 t))))))))
 >
 > `found-1' is useless here (not used).
 >
 > (defun kill-this-buffer-enabled-p ()
 >   (or (not (menu-bar-non-minibuffer-window-p))
 >       (catch 'found
 >         (dolist (buffer  (buffer-list))
 >           (unless (string-match-p "^ " (buffer-name buffer))
 >             (throw 'found t)))
 >         nil)))

The idea is that at least _two_ interesting buffers are needed to enable
`kill-this-buffer'.  found-1 is initially nil and set to t when the
first interesting buffer is found.  The throw to found-2 occurs when the
second interesting buffer has been found.

martin

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[Drew Adams]

>  >> (defun kill-this-buffer-enabled-p ()
>  >>    (or (not (menu-bar-non-minibuffer-window-p))
>  >>        (let (found-1)
>  >> 	(catch 'found-2
>  >> 	  (dolist (buffer (buffer-list))
>  >> 	    (unless (string-match-p "^ " (buffer-name buffer))
>  >> 	      (if (not found-1)
>  >> 		  (setq found-1 t)
>  >> 		(throw 'found-2 t))))))))
> 
> The idea is that at least _two_ interesting buffers are 
> needed to enable `kill-this-buffer'.

Right, that's the existing behavior.

But why?  Why shouldn't menu item `Close' be available to kill the current
buffer even if it is the only "interesting" buffer?  I imagine the answer behind
this design is that we never want to show an uninteresting buffer - or that we
never want to replace an interesting one by an uninteresting one in the same
window.

I don't think that's a good idea.  (I mistakenly thought you were trying to
improve this at the same time as improving the performance - see below.)

`Close' is about killing the buffer.  It is not just or even primarily about
replacing it with another in the window.  I'd say we should let the user kill
the buffer even if it is the only "interesting" one.  A user will wonder (bad
UI) why `Close' isn't available in this case, and even if s?he correctly guesses
why s?he won't necessarily care that there is no other non-interesting buffer to
display.  We should not prevent the user from killing the buffer (via the menu).

Just one opinion.

Another alternative could be to let `Close' be enabled in this case but also
close (delete) the window if the buffer killed was the last "interesting" one.
(But my preference would be to just kill the buffer, even in this case.)

> found-1 is initially nil and set to t when the
> first interesting buffer is found.  The throw to found-2 
> occurs when the second interesting buffer has been found.

Yes, your indentation threw me off (probably from pasting TAB chars).  It looked
like you were throwing in any case (outside the `if').  I thus thought you were
also proposing the behavior improvement of letting the user kill the last
"interesting" buffer.

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

[Stefan Monnier]

>> ??? This function is bound to the "Close" item in the "File" menu, so
>> it is definitely related to the menu bar.

> Am I right that the time for executing `kill-this-buffer-enabled-p' is
> currently always proportional to the number of live buffers?  In that
> case I'd rewrite his function as

> (defun kill-this-buffer-enabled-p ()
>   (or (not (menu-bar-non-minibuffer-window-p))
>       (let (found-1)
> 	(catch 'found-2
> 	  (dolist (buffer (buffer-list))
> 	    (unless (string-match-p "^ " (buffer-name buffer))
> 	      (if (not found-1)
> 		  (setq found-1 t)
> 		(throw 'found-2 t))))))))

Probably a good change, yes.  It'd also be good to add a comment
explaining why we do this loop searching for some non-temp buffer.
IIUC this function is used to check whether kill-this-buffer can do its
job, and if (menu-bar-non-minibuffer-window-p) is nil, then we use
abort-recursive-edit, so we need to check if abort-recursive-edit can be
used, which seems related to minibuffer-depth rather than to
non-temp buffers.


        Stefan

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[martin rudalics]

 >> The idea is that at least _two_ interesting buffers are
 >> needed to enable `kill-this-buffer'.
 >
 > Right, that's the existing behavior.
 >
 > But why?  Why shouldn't menu item `Close' be available to kill the current
 > buffer even if it is the only "interesting" buffer?  I imagine the answer behind
 > this design is that we never want to show an uninteresting buffer - or that we
 > never want to replace an interesting one by an uninteresting one in the same
 > window.

We might end up showing the *code-conversion-work* or *Echo Area* buffer
in a normal window which doesn't strike me as a good idea in response to
invoking a menu item called "Close".

 > I don't think that's a good idea.  (I mistakenly thought you were trying to
 > improve this at the same time as improving the performance - see below.)
 >
 > `Close' is about killing the buffer.  It is not just or even primarily about
 > replacing it with another in the window.  I'd say we should let the user kill
 > the buffer even if it is the only "interesting" one.  A user will wonder (bad
 > UI) why `Close' isn't available in this case, and even if s?he correctly guesses
 > why s?he won't necessarily care that there is no other non-interesting buffer to
 > display.  We should not prevent the user from killing the buffer (via the menu).

I only tried to emulate the current behavior.  Usually, at least the
*scratch* or *Messages* buffer should be around so I suppose that in
practice it's always possible to kill the current buffer.

martin

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

[martin rudalics]

 > It'd also be good to add a comment
 > explaining why we do this loop searching for some non-temp buffer.

Indeed, calling (other-buffer nil t) should be sufficient.

 > IIUC this function is used to check whether kill-this-buffer can do its
 > job, and if (menu-bar-non-minibuffer-window-p) is nil, then we use
 > abort-recursive-edit, so we need to check if abort-recursive-edit can be
 > used, which seems related to minibuffer-depth rather than to
 > non-temp buffers.

To `minibuffer-depth' and `active-minibuffer-window', I suppose.  Still
wondering whether tth is able to trigger his behavior from the menu bar
entry.

martin

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[Drew Adams]

> I only tried to emulate the current behavior.  Usually, at least the
> *scratch* or *Messages* buffer should be around so I suppose that in
> practice it's always possible to kill the current buffer.

Yes, I think so.  And that behavior (always kill) will be less confusing to
users, IMO.  Keep it simple.

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[Johan Bockgård]

martin rudalics <rudalics@gmx.at> writes:

>> But why? Why shouldn't menu item `Close' be available to kill the
>> current buffer even if it is the only "interesting" buffer? I imagine
>> the answer behind this design is that we never want to show an
>> uninteresting buffer - or that we never want to replace an
>> interesting one by an uninteresting one in the same window.
>
> We might end up showing the *code-conversion-work* or *Echo Area* buffer
> in a normal window which doesn't strike me as a good idea in response to
> invoking a menu item called "Close".

That can't happen. The *scratch* buffer is resurrected if all other
visible buffers disappear.

> I only tried to emulate the current behavior.  Usually, at least the
> *scratch* or *Messages* buffer should be around so I suppose that in
> practice it's always possible to kill the current buffer.

It's always possible to kill the current buffer, unless that buffer is
*scratch* and no other visible buffers exist.

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in callsto `kill-this-buffer'

[martin rudalics]

 >> We might end up showing the *code-conversion-work* or *Echo Area* buffer
 >> in a normal window which doesn't strike me as a good idea in response to
 >> invoking a menu item called "Close".
 >
 > That can't happen. The *scratch* buffer is resurrected if all other
 > visible buffers disappear.

I suppose by "visible buffer" you mean the object mentioned in the
comments of `kill-buffer' which is defined as the return value of
`other-buffer'.  That object is mystically unreliable here.  I can
easily crash my trunk by repeatedly killing all buffers.  But I was
never able to provide a presentable scenario so I just went ahead and
fixed this in my branch.

This said you're right that the scenario I described in the text you
cited shouldn't happen because `other-buffer' doesn't return a buffer
whose name starts with a space.

 >> I only tried to emulate the current behavior.  Usually, at least the
 >> *scratch* or *Messages* buffer should be around so I suppose that in
 >> practice it's always possible to kill the current buffer.
 >
 > It's always possible to kill the current buffer, unless that buffer is
 > *scratch* and no other visible buffers exist.

Ideally, yes.

martin

bug#8184: 23.1.90; `menu-bar-non-minibuffer-window-p' error in calls to `kill-this-buffer'

[martin rudalics]

 > Debugger entered--Lisp error: (wrong-type-argument frame-live-p #<dead frame *Help* 0x100d1e930>)
 >   frame-selected-window(#<dead frame *Help* 0x100d1e930>)
 >   menu-bar-non-minibuffer-window-p()
 >   kill-this-buffer()
 >   call-interactively(kill-this-buffer nil nil)

Emacs now in this case doesn't do anything when the associated frame is
not live or visible.  Bug closed.

martin