unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
       [not found] ` <20170412194438.934FD22EE8@vcs0.savannah.gnu.org>
@ 2017-04-12 20:27   ` Stefan Monnier
  2017-04-12 22:03     ` Juri Linkov
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-12 20:27 UTC (permalink / raw)
  To: Juri Linkov; +Cc: Eli Zaretskii, emacs-devel

> +This command can be reliably invoked only from the menu bar,
> +otherwise it could decide to silently do nothing."

All this is due to:

    commit a86b330f8fa754c4b919ea14d0c5dcf261f055c4
    Author: Juri Linkov <juri@jurta.org>
    Date:   Sun Mar 16 17:44:20 2008 +0000
    
        (kill-this-buffer): Use menu-bar-non-minibuffer-window-p
        to check if the current buffer is the minibuffer, and in this case
        call abort-recursive-edit to kill the minibuffer.  Doc fix.
        (kill-this-buffer-enabled-p): Allow this function to return non-nil
        when the current buffer is the minibuffer.
    
    diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
    --- a/lisp/menu-bar.el
    +++ b/lisp/menu-bar.el
    @@ -1444,1 +1446,3 @@
    -  (kill-buffer (current-buffer)))
    +  (if (menu-bar-non-minibuffer-window-p)
    +      (kill-buffer (current-buffer))
    +    (abort-recursive-edit)))

Jury, do you remember why we wanted this behavior?
There are two questions here:
- Why would we want to run abort-recursive-edit when we're in the minibuffer?
- Why did we decide to use menu-bar-non-minibuffer-window-p rather than
  checking something like (window-minibuffer-p (frame-selected-window))?

I think there should be a way to make kill-this-buffer behave sanely
just as much when called from the menu-bar as called from M-x [ I'm not
concerned about calling it from Elisp and I'm perfectly happy to mark it
`interactive-only`.  ]


        Stefan



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-12 20:27   ` [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466) Stefan Monnier
@ 2017-04-12 22:03     ` Juri Linkov
  2017-04-13  6:41       ` Eli Zaretskii
  2017-04-13 18:37       ` Stefan Monnier
  0 siblings, 2 replies; 24+ messages in thread
From: Juri Linkov @ 2017-04-12 22:03 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Eli Zaretskii, emacs-devel

>> +This command can be reliably invoked only from the menu bar,
>> +otherwise it could decide to silently do nothing."
>
> All this is due to:
>
>     commit a86b330f8fa754c4b919ea14d0c5dcf261f055c4
>     Author: Juri Linkov <juri@jurta.org>
>     Date:   Sun Mar 16 17:44:20 2008 +0000
>
>         (kill-this-buffer): Use menu-bar-non-minibuffer-window-p
>         to check if the current buffer is the minibuffer, and in this case
>         call abort-recursive-edit to kill the minibuffer.  Doc fix.
>         (kill-this-buffer-enabled-p): Allow this function to return non-nil
>         when the current buffer is the minibuffer.
>
>     diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
>     --- a/lisp/menu-bar.el
>     +++ b/lisp/menu-bar.el
>     @@ -1444,1 +1446,3 @@
>     -  (kill-buffer (current-buffer)))
>     +  (if (menu-bar-non-minibuffer-window-p)
>     +      (kill-buffer (current-buffer))
>     +    (abort-recursive-edit)))
>
> Jury, do you remember why we wanted this behavior?
> There are two questions here:
> - Why would we want to run abort-recursive-edit when we're in the minibuffer?

I clearly remember that the intention was to discard the minibuffer when
the user clicks on the big cross, i.e. the "Close" button in the toolbar,
because its mnemonics suggests to close the currently active buffer -
if it's the minibuffer, then close the minibuffer.

It also makes sense to do the same when the menu item "Close" is selected
from the "File" menu due to its definition:

    (bindings--define-key menu [kill-buffer]
      '(menu-item "Close" kill-this-buffer
                  :enable (kill-this-buffer-enabled-p)
                  :help "Discard (kill) current buffer"))

where "Discard (kill) current buffer" could also apply to the currently
active minibuffer.

> - Why did we decide to use menu-bar-non-minibuffer-window-p rather than
>   checking something like (window-minibuffer-p (frame-selected-window))?

‘menu-bar-non-minibuffer-window-p’ already does this, and additionally
takes into account ‘menu-updating-frame’ and checks for ‘frame-live-p’.

> I think there should be a way to make kill-this-buffer behave sanely
> just as much when called from the menu-bar as called from M-x [ I'm not
> concerned about calling it from Elisp and I'm perfectly happy to mark it
> `interactive-only`.  ]

In any case it makes no sense to add special-casing for the minibuffer in
‘kill-current-buffer’ since it is necessary only in ‘kill-this-buffer’
associated with the tool-bar/menu-bar.  ‘kill-current-buffer’ should be
just an alias for ‘(kill-buffer (current-buffer))’.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-12 22:03     ` Juri Linkov
@ 2017-04-13  6:41       ` Eli Zaretskii
  2017-04-13 23:35         ` Juri Linkov
  2017-04-13 18:37       ` Stefan Monnier
  1 sibling, 1 reply; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-13  6:41 UTC (permalink / raw)
  To: Juri Linkov; +Cc: monnier, emacs-devel

> From: Juri Linkov <juri@jurta.org>
> Cc: emacs-devel@gnu.org,  Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 13 Apr 2017 01:03:50 +0300
> 
> In any case it makes no sense to add special-casing for the minibuffer in
> ‘kill-current-buffer’ since it is necessary only in ‘kill-this-buffer’
> associated with the tool-bar/menu-bar.

Alas, a couple of users of kill-this-buffer were sometimes calling it
in the context of a menu-invoked command, so that part still can make
sense.  And I have no idea which of the callers relied on this feature
even when invoking the function via non-menu events.  It's been 9
years since this feature was introduced.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-12 22:03     ` Juri Linkov
  2017-04-13  6:41       ` Eli Zaretskii
@ 2017-04-13 18:37       ` Stefan Monnier
  2017-04-13 23:30         ` Juri Linkov
  1 sibling, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-13 18:37 UTC (permalink / raw)
  To: emacs-devel

>> - Why would we want to run abort-recursive-edit when we're in the minibuffer?
> I clearly remember that the intention was to discard the minibuffer when
> the user clicks on the big cross, i.e. the "Close" button in the toolbar,
> because its mnemonics suggests to close the currently active buffer -
> if it's the minibuffer, then close the minibuffer.

I see, thanks, that makes sense, indeed.

>> - Why did we decide to use menu-bar-non-minibuffer-window-p rather than
>> checking something like (window-minibuffer-p (frame-selected-window))?
> ‘menu-bar-non-minibuffer-window-p’ already does this, and additionally
> takes into account ‘menu-updating-frame’ and checks for ‘frame-live-p’.

But that's exactly the question: why pay attention to
menu-updating-frame and  frame-live-p?


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-13 18:37       ` Stefan Monnier
@ 2017-04-13 23:30         ` Juri Linkov
  0 siblings, 0 replies; 24+ messages in thread
From: Juri Linkov @ 2017-04-13 23:30 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

>>> - Why did we decide to use menu-bar-non-minibuffer-window-p rather than
>>> checking something like (window-minibuffer-p (frame-selected-window))?
>> ‘menu-bar-non-minibuffer-window-p’ already does this, and additionally
>> takes into account ‘menu-updating-frame’ and checks for ‘frame-live-p’.
>
> But that's exactly the question: why pay attention to
> menu-updating-frame and  frame-live-p?

menu-bar-non-minibuffer-window-p is used to check whether a menu item
should be enabled, so indeed this is not needed in kill-this-buffer
that should work with just (window-minibuffer-p (frame-selected-window)).

But I still believe that exiting the active minibuffer by kill-this-buffer
makes sense only in the context of menu-bar/tool-bar where the same icon
can be shared to do two different things: either exit the minibuffer
or kill the current buffer.  I can't imagine why anyone would want to do
this with kill-current-buffer bound to the same key when there is already
the special key C-g and also keyboard-escape-quit to exit the minibuffer.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-13  6:41       ` Eli Zaretskii
@ 2017-04-13 23:35         ` Juri Linkov
  2017-04-14  6:35           ` martin rudalics
  2017-04-14  7:27           ` Eli Zaretskii
  0 siblings, 2 replies; 24+ messages in thread
From: Juri Linkov @ 2017-04-13 23:35 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: monnier, emacs-devel

>> In any case it makes no sense to add special-casing for the minibuffer in
>> ‘kill-current-buffer’ since it is necessary only in ‘kill-this-buffer’
>> associated with the tool-bar/menu-bar.
>
> Alas, a couple of users of kill-this-buffer were sometimes calling it
> in the context of a menu-invoked command, so that part still can make
> sense.  And I have no idea which of the callers relied on this feature
> even when invoking the function via non-menu events.  It's been 9
> years since this feature was introduced.

I'm sure that all these callers used kill-this-buffer because there was no
kill-current-buffer to simply kill the current buffer, and nothing more.
So I think we need to leave kill-this-buffer callers only in menu-bar/tool-bar,
and use kill-current-buffer everywhere else, leaving just
‘(kill-buffer (current-buffer))’ in the definition of
kill-current-buffer in simple.el.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-13 23:35         ` Juri Linkov
@ 2017-04-14  6:35           ` martin rudalics
  2017-04-16 21:15             ` Juri Linkov
  2017-04-14  7:27           ` Eli Zaretskii
  1 sibling, 1 reply; 24+ messages in thread
From: martin rudalics @ 2017-04-14  6:35 UTC (permalink / raw)
  To: Juri Linkov, Eli Zaretskii; +Cc: monnier, emacs-devel

 > I'm sure that all these callers used kill-this-buffer because there was no
 > kill-current-buffer to simply kill the current buffer, and nothing more.

Do you mean because it was too tedious to write (kill-buffer nil)?

 > So I think we need to leave kill-this-buffer callers only in menu-bar/tool-bar,
 > and use kill-current-buffer everywhere else, leaving just
 > ‘(kill-buffer (current-buffer))’ in the definition of
 > kill-current-buffer in simple.el.

Why (kill-buffer (current-buffer))?  Isn't (kill-buffer) enough?

martin




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-13 23:35         ` Juri Linkov
  2017-04-14  6:35           ` martin rudalics
@ 2017-04-14  7:27           ` Eli Zaretskii
  2017-04-16 13:42             ` Stefan Monnier
  1 sibling, 1 reply; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-14  7:27 UTC (permalink / raw)
  To: Juri Linkov; +Cc: monnier, emacs-devel

> From: Juri Linkov <juri@jurta.org>
> Cc: monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
> Date: Fri, 14 Apr 2017 02:35:08 +0300
> 
> > Alas, a couple of users of kill-this-buffer were sometimes calling it
> > in the context of a menu-invoked command, so that part still can make
> > sense.  And I have no idea which of the callers relied on this feature
> > even when invoking the function via non-menu events.  It's been 9
> > years since this feature was introduced.
> 
> I'm sure that all these callers used kill-this-buffer because there was no
> kill-current-buffer to simply kill the current buffer, and nothing more.

I'm sure we have no idea whether we can be sure.

After so many years that this code existed, I see no reason to rock
the boat now.  This bike shed's color was fixed long ago; if we didn't
care during all these years, we should definitely leave it at that
now.

All I did was to add a simple sentence to the doc string, something
that was over the years requested a couple of times, with no-one
bothering to do anything, not even reply.  And look what tempest did
this small change cause!  I wonder why.

> So I think we need to leave kill-this-buffer callers only in menu-bar/tool-bar,
> and use kill-current-buffer everywhere else, leaving just
> ‘(kill-buffer (current-buffer))’ in the definition of
> kill-current-buffer in simple.el.

Sorry, I disagree.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-14  7:27           ` Eli Zaretskii
@ 2017-04-16 13:42             ` Stefan Monnier
  2017-04-16 15:21               ` martin rudalics
  2017-04-16 16:54               ` Eli Zaretskii
  0 siblings, 2 replies; 24+ messages in thread
From: Stefan Monnier @ 2017-04-16 13:42 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Juri Linkov, emacs-devel

> After so many years that this code existed, I see no reason to rock
> the boat now.  This bike shed's color was fixed long ago; if we didn't
> care during all these years, we should definitely leave it at that
> now.

I think the patch below would make the function more robust and
eliminate the need to warn about weird behaviors in unusual circumstances.


        Stefan


diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 28464f13df..2ee3f4777d 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -1896,16 +1896,13 @@ menu-bar-non-minibuffer-window-p
 	       (frame-selected-window menu-frame))))))
 
 (defun kill-this-buffer ()	; for the menu bar
-  "Kill the current buffer.
+  "Kill the selected window's buffer.
 When called in the minibuffer, get out of the minibuffer
 using `abort-recursive-edit'."
   (interactive)
   (cond
-   ;; Don't do anything when `menu-frame' is not alive or visible
-   ;; (Bug#8184).
-   ((not (menu-bar-menu-frame-live-and-visible-p)))
-   ((menu-bar-non-minibuffer-window-p)
-    (kill-buffer (current-buffer)))
+   ((window-minibuffer-p (selected-window))
+    (kill-buffer (window-buffer)))
    (t
     (abort-recursive-edit))))
 



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 13:42             ` Stefan Monnier
@ 2017-04-16 15:21               ` martin rudalics
  2017-04-16 19:18                 ` Stefan Monnier
  2017-04-16 16:54               ` Eli Zaretskii
  1 sibling, 1 reply; 24+ messages in thread
From: martin rudalics @ 2017-04-16 15:21 UTC (permalink / raw)
  To: Stefan Monnier, Eli Zaretskii; +Cc: Juri Linkov, emacs-devel

 >     (cond
 > -   ;; Don't do anything when `menu-frame' is not alive or visible
 > -   ;; (Bug#8184).
 > -   ((not (menu-bar-menu-frame-live-and-visible-p)))
 > -   ((menu-bar-non-minibuffer-window-p)
 > -    (kill-buffer (current-buffer)))
 > +   ((window-minibuffer-p (selected-window))
 > +    (kill-buffer (window-buffer)))
 >      (t
 >       (abort-recursive-edit))))

Did you you mean

(if (window-minibuffer-p)
     (abort-recursive-edit)
   (kill-buffer (window-buffer)))

martin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 13:42             ` Stefan Monnier
  2017-04-16 15:21               ` martin rudalics
@ 2017-04-16 16:54               ` Eli Zaretskii
  2017-04-16 19:22                 ` Stefan Monnier
  1 sibling, 1 reply; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-16 16:54 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: juri, emacs-devel

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Juri Linkov <juri@jurta.org>, emacs-devel@gnu.org
> Date: Sun, 16 Apr 2017 09:42:04 -0400
> 
> > After so many years that this code existed, I see no reason to rock
> > the boat now.  This bike shed's color was fixed long ago; if we didn't
> > care during all these years, we should definitely leave it at that
> > now.
> 
> I think the patch below would make the function more robust and
> eliminate the need to warn about weird behaviors in unusual circumstances.

Thanks, but I'm not interested.  This code existed for years, and it
never bothered you or anyone else, so we won't change it now.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 15:21               ` martin rudalics
@ 2017-04-16 19:18                 ` Stefan Monnier
  2017-04-16 20:37                   ` martin rudalics
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-16 19:18 UTC (permalink / raw)
  To: emacs-devel

> (if (window-minibuffer-p)
>     (abort-recursive-edit)
>   (kill-buffer (window-buffer)))

Oh, indeed, we can drop the (selected-window) argument to
window-minibuffer-p.


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 16:54               ` Eli Zaretskii
@ 2017-04-16 19:22                 ` Stefan Monnier
  2017-04-16 20:07                   ` Eli Zaretskii
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-16 19:22 UTC (permalink / raw)
  To: emacs-devel

> Thanks, but I'm not interested.  This code existed for years, and it
> never bothered you or anyone else, so we won't change it now.

It's a (cleaner) alternative to the fix below:

    commit 96ef9ccd1b0a28c774ad5b9ffbfb811de540eb30
    Author: Martin Rudalics <rudalics@gmx.at>
    Date:   Wed Oct 3 10:50:49 2012 +0200
    
        Have kill-this-buffer don't do anything when frame is not alive or visible (Bug#8184).
        
        * menu-bar.el (kill-this-buffer): Don't do anything when
        `menu-frame' is not alive or visible (Bug#8184).

so, yes, it's been around for a few years but it was pretty ugly.


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 19:22                 ` Stefan Monnier
@ 2017-04-16 20:07                   ` Eli Zaretskii
  2017-04-17  3:00                     ` Stefan Monnier
  0 siblings, 1 reply; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-16 20:07 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 16 Apr 2017 15:22:53 -0400
> 
> > Thanks, but I'm not interested.  This code existed for years, and it
> > never bothered you or anyone else, so we won't change it now.
> 
> It's a (cleaner) alternative to the fix below:
> 
>     commit 96ef9ccd1b0a28c774ad5b9ffbfb811de540eb30
>     Author: Martin Rudalics <rudalics@gmx.at>
>     Date:   Wed Oct 3 10:50:49 2012 +0200
>     
>         Have kill-this-buffer don't do anything when frame is not alive or visible (Bug#8184).
>         
>         * menu-bar.el (kill-this-buffer): Don't do anything when
>         `menu-frame' is not alive or visible (Bug#8184).
> 
> so, yes, it's been around for a few years but it was pretty ugly.

We (and you in particular) have lived with that "ugliness" for almost
5 years, so I guess it's not that ugly after all.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 19:18                 ` Stefan Monnier
@ 2017-04-16 20:37                   ` martin rudalics
  0 siblings, 0 replies; 24+ messages in thread
From: martin rudalics @ 2017-04-16 20:37 UTC (permalink / raw)
  To: Stefan Monnier, emacs-devel

 >> (if (window-minibuffer-p)
 >>      (abort-recursive-edit)
 >>    (kill-buffer (window-buffer)))
 >
 > Oh, indeed, we can drop the (selected-window) argument to
 > window-minibuffer-p.

Your patch amounted to

(if (window-minibuffer-p)
     (kill-buffer (window-buffer))
   (abort-recursive-edit))

which didn't strike me as very useful.

martin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-14  6:35           ` martin rudalics
@ 2017-04-16 21:15             ` Juri Linkov
  0 siblings, 0 replies; 24+ messages in thread
From: Juri Linkov @ 2017-04-16 21:15 UTC (permalink / raw)
  To: martin rudalics; +Cc: Eli Zaretskii, monnier, emacs-devel

>> I'm sure that all these callers used kill-this-buffer because there was no
>> kill-current-buffer to simply kill the current buffer, and nothing more.
>
> Do you mean because it was too tedious to write (kill-buffer nil)?

Yes, as a command it is easier to use kill-current-buffer from M-x
or bind it to a key.

>> So I think we need to leave kill-this-buffer callers only in menu-bar/tool-bar,
>> and use kill-current-buffer everywhere else, leaving just
>> ‘(kill-buffer (current-buffer))’ in the definition of
>> kill-current-buffer in simple.el.
>
> Why (kill-buffer (current-buffer))?  Isn't (kill-buffer) enough?

(kill-buffer (current-buffer)) is more explicit.  I think there are other
parts of kill-this-buffer/kill-current-buffer that need to be simplified.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-16 20:07                   ` Eli Zaretskii
@ 2017-04-17  3:00                     ` Stefan Monnier
  2017-04-17  5:39                       ` Eli Zaretskii
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-17  3:00 UTC (permalink / raw)
  To: emacs-devel

> We (and you in particular) have lived with that "ugliness" for almost
> 5 years, so I guess it's not that ugly after all.

By this measure, we can stop hacking on Emacs,


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17  3:00                     ` Stefan Monnier
@ 2017-04-17  5:39                       ` Eli Zaretskii
  2017-04-17 12:14                         ` Stefan Monnier
  2017-04-17 14:25                         ` Richard Stallman
  0 siblings, 2 replies; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-17  5:39 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 16 Apr 2017 23:00:23 -0400
> 
> > We (and you in particular) have lived with that "ugliness" for almost
> > 5 years, so I guess it's not that ugly after all.
> 
> By this measure, we can stop hacking on Emacs,

Not hacking, only bike-shedding.

Stopping the bike-shedding would be nice, but I don't expect that to
happen in my lifetime.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17  5:39                       ` Eli Zaretskii
@ 2017-04-17 12:14                         ` Stefan Monnier
  2017-04-17 14:35                           ` Eli Zaretskii
  2017-04-17 14:25                         ` Richard Stallman
  1 sibling, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-17 12:14 UTC (permalink / raw)
  To: emacs-devel

>> By this measure, we can stop hacking on Emacs,
> Not hacking, only bike-shedding.

To me bike-shedding means that it's only an aesthetic issue, but in this
case the ugliness has enough real consequences that you felt compelled
to document them via:

    diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
    index 208f4c2..1594270 100644
    --- a/lisp/menu-bar.el
    +++ b/lisp/menu-bar.el
    @@ -1904,7 +1904,10 @@ updating the menu."
     (defun kill-this-buffer ()	; for the menu bar
       "Kill the current buffer.
     When called in the minibuffer, get out of the minibuffer
    -using `abort-recursive-edit'."
    +using `abort-recursive-edit'.
    +
    +This command can be reliably invoked only from the menu bar,
    +otherwise it could decide to silently do nothing."
       (interactive)
       (cond
        ;; Don't do anything when `menu-frame' is not alive or visible

My patch (as fixed by Martin) fixes the problem instead of documenting
the resulting quirks.  While it's not terribly important, it's not
just cosmetic.


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17  5:39                       ` Eli Zaretskii
  2017-04-17 12:14                         ` Stefan Monnier
@ 2017-04-17 14:25                         ` Richard Stallman
  1 sibling, 0 replies; 24+ messages in thread
From: Richard Stallman @ 2017-04-17 14:25 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: monnier, emacs-devel

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

If we shed all our bikes on this point, it would take us some time
to acquire more bikes.  During that time, we couldn't shed any more.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17 12:14                         ` Stefan Monnier
@ 2017-04-17 14:35                           ` Eli Zaretskii
  2017-04-17 16:05                             ` Stefan Monnier
  0 siblings, 1 reply; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-17 14:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 17 Apr 2017 08:14:58 -0400
> 
> >> By this measure, we can stop hacking on Emacs,
> > Not hacking, only bike-shedding.
> 
> To me bike-shedding means that it's only an aesthetic issue, but in this
> case the ugliness has enough real consequences that you felt compelled
> to document them via:

The request for documenting the subtlety was voiced more than once
over the years, and I saw no reason not to act on that.  We document
similar limitations and subtleties all over the place, so there's
nothing extraordinary in this case.

And I see no ugliness here, so if it exists, it must be minor.  Thus
it makes very little sense to me to insist on changing code that
existed for several years because of some ugliness on whose existence
we cannot even agree.

> My patch (as fixed by Martin) fixes the problem instead of documenting
> the resulting quirks.

I think your patch involves some real, though perhaps minor, risk of
breaking some code out there, and I don't think that risk is
justified, given the (un)importance of the issue.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17 14:35                           ` Eli Zaretskii
@ 2017-04-17 16:05                             ` Stefan Monnier
  2017-04-17 16:52                               ` Eli Zaretskii
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier @ 2017-04-17 16:05 UTC (permalink / raw)
  To: emacs-devel

> The request for documenting the subtlety was voiced more than once
> over the years,

IOW, people have complained about that buggy behavior.

> and I saw no reason not to act on that.

Agreed.  Documenting the bug seems like a poorer choice than fixing it, tho.

> We document similar limitations and subtleties all over the place, so
> there's nothing extraordinary in this case.

The difference is that we know how to fix the bug rather than document it.

> And I see no ugliness here, so if it exists, it must be minor.

The ugliness is that the kill-this-buffer code uses
menu-bar-non-minibuffer-window-p and
menu-bar-menu-frame-live-and-visible-p both of which consult
menu-updating-frame because they are designed to be used in menu
attributes (as in :enable, :active, ...).

This misuse implies undesirable behavior when menu-updating-frame
happens to have a non-nil value when the command gets run (e.g. bug#8184
where the "fix" just made the command do nothing instead of making it do
what it should do).

So this whole problem is due to a mistake in the original code which
used menu-bar-non-minibuffer-window-p instead of window-minibuffer-p,
without realizing the difference.

It's a plain bug.  Minor, but a bug nonetheless.


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17 16:05                             ` Stefan Monnier
@ 2017-04-17 16:52                               ` Eli Zaretskii
  2017-04-19 12:12                                 ` Stefan Monnier
  0 siblings, 1 reply; 24+ messages in thread
From: Eli Zaretskii @ 2017-04-17 16:52 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 17 Apr 2017 12:05:02 -0400
> 
> > The request for documenting the subtlety was voiced more than once
> > over the years,
> 
> IOW, people have complained about that buggy behavior.

They used the function outside its domain, because its domain wasn't
documented.

> The difference is that we know how to fix the bug rather than document it.

No, we _think_ we know how to fix the "bug", and we _think_ the fix
won't cause other problems.

> > And I see no ugliness here, so if it exists, it must be minor.
> 
> The ugliness is that the kill-this-buffer code uses
> menu-bar-non-minibuffer-window-p and
> menu-bar-menu-frame-live-and-visible-p both of which consult
> menu-updating-frame because they are designed to be used in menu
> attributes (as in :enable, :active, ...).

There's no ugliness: this function was designed to be used from a menu
bar.  It took several iterations over several years to get it right
with all the various GUI variants we support.  It wasn't easy.  Since
its last change, it worked without any complaints, except about its
being inappropriate outside of menus.  And yet you still want to make
these changes, in the very same place which took us years to get
right.  What makes you think we are now any wiser than we were back
then, and won't be making the same kinds of mistakes we did then?

The master branch now has a new kill-current-buffer function that
doesn't refer to menus, under a different name.  New code should use
it in preference to kill-this-buffer.  With luck, uses of
kill-this-buffer outside of the menus will stop now.  And we already
know kill-this-buffer is okay when invoked from menus.  So I think
this particular bike shed has the right color, and we can move on to
more serious issues.

> It's a plain bug.  Minor, but a bug nonetheless.

What can I say? I disagree.  From my POV, it's a minor issue not worth
the risk of breaking users of this function.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)
  2017-04-17 16:52                               ` Eli Zaretskii
@ 2017-04-19 12:12                                 ` Stefan Monnier
  0 siblings, 0 replies; 24+ messages in thread
From: Stefan Monnier @ 2017-04-19 12:12 UTC (permalink / raw)
  To: emacs-devel

>> The ugliness is that the kill-this-buffer code uses
>> menu-bar-non-minibuffer-window-p and
>> menu-bar-menu-frame-live-and-visible-p both of which consult
>> menu-updating-frame because they are designed to be used in menu
>> attributes (as in :enable, :active, ...).
> There's no ugliness: this function was designed to be used from a menu
> bar.

menu-bar-non-minibuffer-window-p is for use in menu-bar item
attributes that are evaluated while building the menu-bar itself.

It shouldn't be used in commands, even those that are bound to menu-bar
events.


        Stefan




^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2017-04-19 12:12 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20170412194437.19648.75020@vcs0.savannah.gnu.org>
     [not found] ` <20170412194438.934FD22EE8@vcs0.savannah.gnu.org>
2017-04-12 20:27   ` [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466) Stefan Monnier
2017-04-12 22:03     ` Juri Linkov
2017-04-13  6:41       ` Eli Zaretskii
2017-04-13 23:35         ` Juri Linkov
2017-04-14  6:35           ` martin rudalics
2017-04-16 21:15             ` Juri Linkov
2017-04-14  7:27           ` Eli Zaretskii
2017-04-16 13:42             ` Stefan Monnier
2017-04-16 15:21               ` martin rudalics
2017-04-16 19:18                 ` Stefan Monnier
2017-04-16 20:37                   ` martin rudalics
2017-04-16 16:54               ` Eli Zaretskii
2017-04-16 19:22                 ` Stefan Monnier
2017-04-16 20:07                   ` Eli Zaretskii
2017-04-17  3:00                     ` Stefan Monnier
2017-04-17  5:39                       ` Eli Zaretskii
2017-04-17 12:14                         ` Stefan Monnier
2017-04-17 14:35                           ` Eli Zaretskii
2017-04-17 16:05                             ` Stefan Monnier
2017-04-17 16:52                               ` Eli Zaretskii
2017-04-19 12:12                                 ` Stefan Monnier
2017-04-17 14:25                         ` Richard Stallman
2017-04-13 18:37       ` Stefan Monnier
2017-04-13 23:30         ` Juri Linkov

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).