Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

> +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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Juri Linkov]

>> +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))’.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Juri Linkov]

>> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[martin rudalics]

 > 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Juri Linkov]

>> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

> 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))))
 

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[martin rudalics]

 >     (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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

> (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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[martin rudalics]

 >> (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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

> 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

> 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

>> 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

> 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

>> 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Richard Stallman]

[[[ 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.

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Stefan Monnier]

>> - 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

Re: [Emacs-diffs] emacs-25 bc55a57: * lisp/menu-bar.el (kill-this-buffer): Doc fix. (Bug#26466)

[Juri Linkov]

>>> - 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.