bug#38294: [PATCH] Handle killing of virtual buffers in Ido

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Dario Gjorgjevski]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

Currently, ‘ido-kill-buffer’ does essentially nothing to virtual
buffers.  (They are visited and then immediately killed.)  The attached
patch changes that so that they are instead removed from ‘recentf-list’,
akin to ‘ido-kill-buffer-at-head’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Handle killing of virtual buffers in Ido --]
[-- Type: text/x-diff, Size: 1374 bytes --]

From 4295cac14ea280d43c0aa493520543aa403a4c2e Mon Sep 17 00:00:00 2001
From: Dario Gjorgjevski <dario.gjorgjevski+git@gmail.com>
Date: Wed, 20 Nov 2019 09:34:58 +0100
Subject: [PATCH] Handle killing of virtual buffers in Ido
To: bug-gnu-emacs@gnu.org

* lisp/ido.el (ido-buffer-internal): Handle killing of virtual buffers
as a special case.
(ido-visit-buffer): Document the special case.
---
 lisp/ido.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 79f259b819..bf3f57883d 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2286,7 +2286,10 @@ ido-buffer-internal
        ((and ido-enable-virtual-buffers
 	     ido-virtual-buffers
 	     (setq filename (assoc buf ido-virtual-buffers)))
-	(ido-visit-buffer (find-file-noselect (cdr filename)) method t))
+        (if (eq method 'kill)
+            (setq recentf-list
+	          (delete (cdr filename) recentf-list))
+	  (ido-visit-buffer (find-file-noselect (cdr filename)) method t)))
 
        ((and (eq ido-create-new-buffer 'prompt)
 	     (null require-match)
@@ -4128,6 +4131,8 @@ ido-visit-buffer
       (setq buffer (buffer-name buffer)))
   (let (win newframe)
     (cond
+     ;; "killing" of virtual buffers is handled in
+     ;; `ido-buffer-internal'
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-- 
2.17.1

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

> Currently, ‘ido-kill-buffer’ does essentially nothing to virtual
> buffers.  (They are visited and then immediately killed.)  The attached
> patch changes that so that they are instead removed from ‘recentf-list’,
> akin to ‘ido-kill-buffer-at-head’.

I noticed that you have gotten no reply here.  Could you please
describe what problem it is you are solving, is this fixing a bug or
adding a new feature?

How can I test that this code works as advertised, preferably starting
from "emacs -Q"?  Please give a step by step description, including a
description of what happens before and after applying your patch.

Thanks in advance.

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Dario Gjorgjevski]

Hi Stefan,

Thanks a lot for getting back to me.

> I noticed that you have gotten no reply here.  Could you please
> describe what problem it is you are solving, is this fixing a bug or
> adding a new feature?

I would say this neither fixes a bug nor introduces a new feature;
merely makes ‘ido-kill-buffer’ behave as I expect it to.

> How can I test that this code works as advertised, preferably starting
> from "emacs -Q"?  Please give a step by step description, including a
> description of what happens before and after applying your patch.

What follows is a way to reproduce this scenario, assuming the file
‘foo’ does not exist.

Start by

  M-x recentf-mode <RET>
  M-x ido-mode <RET>
  M-: (setq ido-use-virtual-buffers t) <RET>
  C-x C-f foo <RET> <RET>
  C-x C-s
  C-x k <RET>

Now, ‘foo’ is a _virtual buffer_ in the terminology of Ido, and since we
have ‘ido-use-virtual-buffers’ set to t, we should be able to operate on
it: switch to it or kill it.  Switching works fine; however, *killing is
a no-op*.

At this point, when you do

  C-x k foo <RET>

you will notice that ‘foo’ remains in the list of buffers, despite being
“killed.”  You will still see when doing C-x b or C-x k.  In fact, you
can repeat the above step ad infinitum.

What this patch changes is exactly this behavior: once a virtual buffer
has been “killed” from C-x k, it no longer appears in the list of
buffers.

Let me know if this makes sense.

Best regards,
Dario

-- 
dario.gjorgjevski@gmail.com :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Hi Dario,

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

> What follows is a way to reproduce this scenario, assuming the file
> ‘foo’ does not exist.
>
> Start by
>
>   M-x recentf-mode <RET>
>   M-x ido-mode <RET>
>   M-: (setq ido-use-virtual-buffers t) <RET>
>   C-x C-f foo <RET> <RET>
>   C-x C-s
>   C-x k <RET>
>
> Now, ‘foo’ is a _virtual buffer_ in the terminology of Ido, and since we
> have ‘ido-use-virtual-buffers’ set to t, we should be able to operate on
> it: switch to it or kill it.  Switching works fine; however, *killing is
> a no-op*.
>
> At this point, when you do
>
>   C-x k foo <RET>
>
> you will notice that ‘foo’ remains in the list of buffers, despite being
> “killed.”  You will still see when doing C-x b or C-x k.  In fact, you
> can repeat the above step ad infinitum.

Thank you for the detailed explanation.

> What this patch changes is exactly this behavior: once a virtual buffer
> has been “killed” from C-x k, it no longer appears in the list of
> buffers.
>
> Let me know if this makes sense.

I think the current behaviour makes sense, but I can very much see
that some users would want what you suggest.  Both choices here are
valid.

Given that Ido has behaved like this for a long time, I would
be wary of changing the default behaviour at this point.  It risks
surprising users.

My suggestion would theferefore be to add a new option to enable the
behaviour you suggest.  It could perhaps be named something like
ido-kill-removes-virtual-buffers, and should be disabled by default.

What do you think?

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

>> What this patch changes is exactly this behavior: once a virtual buffer
>> has been “killed” from C-x k, it no longer appears in the list of
>> buffers.
>>
>> Let me know if this makes sense.
>
> I think the current behaviour makes sense, but I can very much see
> that some users would want what you suggest.  Both choices here are
> valid.

I think so, too, but I don't use this combination of features, so I'm
not sure.

It does seem natural that killing a buffer would remove it from the list
of buffers ido-mode offers up, though, so I'm not totally sure whether
any ido users really expect this behaviour, or if they've just lived
with it as a bug.

Any opinions here?

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

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>>> What this patch changes is exactly this behavior: once a virtual buffer
>>> has been “killed” from C-x k, it no longer appears in the list of
>>> buffers.
>>>
>>> Let me know if this makes sense.
>>
>> I think the current behaviour makes sense, but I can very much see
>> that some users would want what you suggest.  Both choices here are
>> valid.
>
> I think so, too, but I don't use this combination of features, so I'm
> not sure.
>
> It does seem natural that killing a buffer would remove it from the list
> of buffers ido-mode offers up, though, so I'm not totally sure whether
> any ido users really expect this behaviour, or if they've just lived
> with it as a bug.
>
> Any opinions here?

The idea specifically with ido-use-virtual-buffers (from reading its doc
string, not from being a user) is that one shouldn't have to worry about
whether or not the buffer is open or not.  To my mind, that implies that
it means that even when manually killing buffers, I should still be able
to switch to it.

(It also seems a bit too surprising to now change the long-standing
default behaviour.)

However, if some users specifically want this behaviour only for
auto-cleaned buffers (e.g. by midnight), I don't see why we couldn't
provide it optionally.

Anyways, I don't know if there is anything useful in the above or if I'm
just rambling.  I agree that it would be good to hear the opinion of
someone else who actually uses this.

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> The idea specifically with ido-use-virtual-buffers (from reading its doc
> string, not from being a user) is that one shouldn't have to worry about
> whether or not the buffer is open or not.  To my mind, that implies that
> it means that even when manually killing buffers, I should still be able
> to switch to it.
>
> (It also seems a bit too surprising to now change the long-standing
> default behaviour.)

Yeah, that sounds reasonable.  The only question is whether manual
killing should be handled differently than automatic killing (i.e., by
midnight.el).

But...  that would be a kinda finicky interface, with C-x k working one
way, and indirect calls to it another.

So I agree with you that this is probably not something we want....

> However, if some users specifically want this behaviour only for
> auto-cleaned buffers (e.g. by midnight), I don't see why we couldn't
> provide it optionally.

An option for something as obscure as this seems like overkill.

> Anyways, I don't know if there is anything useful in the above or if I'm
> just rambling.  I agree that it would be good to hear the opinion of
> someone else who actually uses this.

It would.  :-)

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

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> An option for something as obscure as this seems like overkill.

Agreed.

>> Anyways, I don't know if there is anything useful in the above or if I'm
>> just rambling.  I agree that it would be good to hear the opinion of
>> someone else who actually uses this.
>
> It would.  :-)

I guess we could give it N more weeks or days for anyone to chime in
before closing this as wontfix.

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I guess we could give it N more weeks or days for anyone to chime in
> before closing this as wontfix.

Yup.

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

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Dario Gjorgjevski]

Hi Stefan, hi Lars,

> The idea specifically with ido-use-virtual-buffers (from reading its doc
> string, not from being a user) is that one shouldn't have to worry about
> whether or not the buffer is open or not.  To my mind, that implies that
> it means that even when manually killing buffers, I should still be able
> to switch to it.

I agree with this.  In that case, should we prevent virtual buffers from
being listed in ‘ido-kill-buffer’ in the first place?  Selecting one of
them to be “killed” results in a no-op anyway, so why give people the
impression that something happens?

Best regards,
Dario

-- 
dario.gjorgjevski@gmail.com :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

> I agree with this.  In that case, should we prevent virtual buffers from
> being listed in ‘ido-kill-buffer’ in the first place?  Selecting one of
> them to be “killed” results in a no-op anyway, so why give people the
> impression that something happens?

Killing it isn't a no-op though.  When you kill it, you get prompted to
save changes, it runs hooks in the background, presumably removes it
from active memory, other functions will no longer be able to just
switch to it, etc., etc.

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Dario Gjorgjevski]

Hi Stefan,

> Killing it isn't a no-op though.  When you kill it, you get prompted to
> save changes, it runs hooks in the background, presumably removes it
> from active memory, other functions will no longer be able to just
> switch to it, etc., etc.

Point taken.  In that case, my only complaint would be that
‘ido-kill-buffer-at-head’ does treat killing a virtual buffer as
removing it from ‘recentf-list’ (just as suggested here), so there is
some inconsistency.  However, I can live with it.

Best regards,
Dario

-- 
dario.gjorgjevski@gmail.com :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Lars Ingebrigtsen]

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

> Hi Stefan,
>
>> Killing it isn't a no-op though.  When you kill it, you get prompted to
>> save changes, it runs hooks in the background, presumably removes it
>> from active memory, other functions will no longer be able to just
>> switch to it, etc., etc.

Yeah, but these "virtual buffers" don't really exist, so none of that
will happen, I'd have thought?

> Point taken.  In that case, my only complaint would be that
> ‘ido-kill-buffer-at-head’ does treat killing a virtual buffer as
> removing it from ‘recentf-list’ (just as suggested here), so there is
> some inconsistency.  However, I can live with it.

Yeah, that is inconsistent:

;;; KILL CURRENT BUFFER
(defun ido-kill-buffer-at-head ()
  "Kill the buffer at the head of `ido-matches'.
If cursor is not at the end of the user input, delete to end of input."
[...]
       ;; Handle virtual buffers
       ((assoc buf ido-virtual-buffers)
	(setq recentf-list
	      (delete (cdr (assoc buf ido-virtual-buffers)) recentf-list))

So now I'm kinda thinking that your original patch is correct.  :-)

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

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>> Killing it isn't a no-op though.  When you kill it, you get prompted to
>>> save changes, it runs hooks in the background, presumably removes it
>>> from active memory, other functions will no longer be able to just
>>> switch to it, etc., etc.
>
> Yeah, but these "virtual buffers" don't really exist, so none of that
> will happen, I'd have thought?

But the virtual buffers are only visible on `C-x C-f', not on `C-x b',
right?

At least, that's what it looks like when I do:

0. emacs -Q
1. M-x ido-mode
2. (setq ido-virtual-buffers t)
3. C-x C-f existing-file-foo RET
4. C-x k RET
5. C-x k      => existing-file-foo is not shown

But I may very well be overlooking something...

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Dario Gjorgjevski]

> But the virtual buffers are only visible on `C-x C-f', not on `C-x b',
> right?

They’re visible everywhere: also on ‘C-x b’ and ‘C-x k’.  The latter is
what this patch is related to.

> 2. (setq ido-virtual-buffers t)

The correct variable is ‘ido-use-virtual-buffers’, not
‘ido-virtual-buffers’.

Best regards,
Dario

-- 
dario.gjorgjevski@gmail.com :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

>> But the virtual buffers are only visible on `C-x C-f', not on `C-x b',
>> right?
>
> They’re visible everywhere: also on ‘C-x b’ and ‘C-x k’.  The latter is
> what this patch is related to.
>
>> 2. (setq ido-virtual-buffers t)
>
> The correct variable is ‘ido-use-virtual-buffers’, not
> ‘ido-virtual-buffers’.

Uhm, okay, yes that explains why I didn't see it...

But my question is then why this buffer even shows up on `C-x k' if it's
a virtual buffer.  Instead of making it do something on kill, isn't it
better to just never show it if it's not actually open?

Best regards,
Stefan Kangas

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Dario Gjorgjevski]

> But my question is then why this buffer even shows up on `C-x k' if it's
> a virtual buffer.  Instead of making it do something on kill, isn't it
> better to just never show it if it's not actually open?

That’s exactly why I asked “In that case, should we prevent virtual
buffers from being listed in ‘ido-kill-buffer’ in the first place?”
before.

To me, both _not offering virtual buffers to be killed at all_ and
_offering them but treating killing as removing from ‘recentf-list’_
make sense.  The latter I prefer simply for consistency with
‘ido-kill-buffer-at-head’ (i.e., function that is called when you press
‘C-k’ on a buffer during ‘C-x b’).

Best regards,
Dario

-- 
dario.gjorgjevski@gmail.com :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Lars Ingebrigtsen]

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

> To me, both _not offering virtual buffers to be killed at all_ and
> _offering them but treating killing as removing from ‘recentf-list’_
> make sense.  The latter I prefer simply for consistency with
> ‘ido-kill-buffer-at-head’ (i.e., function that is called when you press
> ‘C-k’ on a buffer during ‘C-x b’).

Yeah, I think you're right, and the original patch was the correct
solution here: When the user explicitly (and interactively) kills a
buffer, then they expect that buffer to go away.  (There isn't any other
reasonable interpretation for that interactive action.)

It's perhaps slightly inconsistent, but I think it's more DWIM.

So I'm applying your patch to Emacs 28, and we'll see if any users of
this ido setting complains (in which case we should be prepared to
revert it).

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

bug#38294: [PATCH] Handle killing of virtual buffers in Ido

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yeah, I think you're right, and the original patch was the correct
> solution here: When the user explicitly (and interactively) kills a
> buffer, then they expect that buffer to go away.  (There isn't any other
> reasonable interpretation for that interactive action.)
>
> It's perhaps slightly inconsistent, but I think it's more DWIM.

Thanks!