bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Leo]

The original change was to address buffer name changes due to packages
such as uniquify.el. But it causes another annoying bug: changing the
order of matches seen by users.

This reverts it.

If people are annoyed (which I doubt) by outdated buffer names due to
uniquify.el, one solution is to map the buffer-names to buffer objects
before killing and map them back to names after killing.

From 7e6597c54a7764688855c3ab2efa6cfa1cffbea6 Mon Sep 17 00:00:00 2001
Date: Mon, 18 Oct 2010 01:44:24 +0800
Subject: [PATCH] Don't rebuild buffer list in iswitchb-visit-buffer

Rebuilding buffer list will lose the order of matches seen by users
and thus cause surprises.
---
 lisp/iswitchb.el |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lisp/iswitchb.el b/lisp/iswitchb.el
index 081897a..b7baa03 100644
--- a/lisp/iswitchb.el
+++ b/lisp/iswitchb.el
@@ -1042,10 +1042,8 @@ Return the modified list with the last element prepended to it."
 	  (if (get-buffer buf)
 	      ;; buffer couldn't be killed.
 	      (setq iswitchb-rescan t)
-	    ;; Else `kill-buffer' succeeds so re-make the buffer list
-	    ;; taking into account packages like uniquify may rename
-	    ;; buffers
-	    (iswitchb-make-buflist iswitchb-default))))))
+	    ;; else buffer was killed so remove name from list.
+	    (setq iswitchb-buflist  (delq buf iswitchb-buflist)))))))
 
 ;;; VISIT CHOSEN BUFFER
 (defun iswitchb-visit-buffer (buffer)
-- 
1.7.3

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Óscar Fuentes]

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

> The original change was to address buffer name changes due to packages
> such as uniquify.el. But it causes another annoying bug: changing the
> order of matches seen by users.
>
> This reverts it.
>
> If people are annoyed (which I doubt) by outdated buffer names due to
> uniquify.el,

The purpose of the patch you reverted was addressing that annoyance, so
it is easy to infer that someone indeed was annoyed by that behavior.

> one solution is to map the buffer-names to buffer objects
> before killing and map them back to names after killing.

So why don't you implement that instead of reverting a patch that
corrects a bug? (After killing a buffer, having a list containing
non-existent buffers is a bug, right? while seeing how the list after
the second item is reordered is an annoyance, so you are proposing to
re-introduce a bug for avoiding an annoyance)

[snip]

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Stefan Monnier]

> The original change was to address buffer name changes due to packages
> such as uniquify.el. But it causes another annoying bug: changing the
> order of matches seen by users.

> This reverts it.

I don't think we should just revert.  If the fix for the old problem
introduces a new problem, we should try and find a solution that fixes
both problems at once.


        Stefan

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Leo]

On 2010-10-18 02:22 +0800, Óscar Fuentes wrote:
> The purpose of the patch you reverted was addressing that annoyance, so
> it is easy to infer that someone indeed was annoyed by that behavior.
[...]
>
> So why don't you implement that instead of reverting a patch that
> corrects a bug? (After killing a buffer, having a list containing
> non-existent buffers is a bug, right? while seeing how the list after
> the second item is reordered is an annoyance, so you are proposing to
> re-introduce a bug for avoiding an annoyance)

On 2010-10-18 02:48 +0800, Stefan Monnier wrote:
> I don't think we should just revert.  If the fix for the old problem
> introduces a new problem, we should try and find a solution that fixes
> both problems at once.
>
>         Stefan

Sorry for being too lazy. Please try the following patch.

From d61fe17fff2926731ea317b21d647a4cf2d136f4 Mon Sep 17 00:00:00 2001
Date: Mon, 18 Oct 2010 01:44:24 +0800
Subject: [PATCH] Rebuild buffer list using buffer objects

in iswitchb-kill-buffer.

Avoid `iswitchb-make-buflist' which changes the order of matches seen
by users.
---
 lisp/iswitchb.el |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/lisp/iswitchb.el b/lisp/iswitchb.el
index 081897a..4cab5ee 100644
--- a/lisp/iswitchb.el
+++ b/lisp/iswitchb.el
@@ -1033,7 +1033,9 @@ Return the modified list with the last element prepended to it."
     (setq buf (car iswitchb-matches))
     ;; check to see if buf is non-nil.
     (if buf
-	(progn
+	(let ((bufobjs (mapcar (lambda (name)
+				 (or (get-buffer name) name))
+			       iswitchb-buflist)))
 	  (kill-buffer buf)
 
 	  ;; Check if buffer exists.  XEmacs gnuserv.el makes alias
@@ -1042,10 +1044,13 @@ Return the modified list with the last element prepended to it."
 	  (if (get-buffer buf)
 	      ;; buffer couldn't be killed.
 	      (setq iswitchb-rescan t)
-	    ;; Else `kill-buffer' succeeds so re-make the buffer list
-	    ;; taking into account packages like uniquify may rename
-	    ;; buffers
-	    (iswitchb-make-buflist iswitchb-default))))))
+	    ;; else buffer was killed
+	    (setq iswitchb-buflist
+		  (delq nil (mapcar (lambda (b)
+				      (if (bufferp b)
+					  (buffer-name b)
+					b))
+				    bufobjs))))))))
 
 ;;; VISIT CHOSEN BUFFER
 (defun iswitchb-visit-buffer (buffer)
-- 
1.7.3

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Stefan Monnier]

> -	    ;; Else `kill-buffer' succeeds so re-make the buffer list
> -	    ;; taking into account packages like uniquify may rename
> -	    ;; buffers
> -	    (iswitchb-make-buflist iswitchb-default))))))
> +	    ;; else buffer was killed
> +	    (setq iswitchb-buflist
> +		  (delq nil (mapcar (lambda (b)
> +				      (if (bufferp b)
> +					  (buffer-name b)
> +					b))
> +				    bufobjs))))))))
 
I was about to install that change when I realized that this is
fundamentally not the right approach: since some of the buffers may have
changed name, the new list of matching buffers may be different (some
buffers that didn't match before may match now and vice-versa).

So iswitchb-make-buflist is more correct.  To deal with the problem of
ordering, we'll need to combine the two: call iswitchb-make-buflist to
get the new list of matches, and then use bufobjs to sort the new
iswitchb-buflist.

Something like

	    (iswitchb-make-buflist iswitchb-default))))))
	    ;; Try to preserve the previous sort order.
	    (setq iswitchb-buflist
		  (sort iswitchb-buflist
                        (lambda (bn1 bn2)
                          (< (length (or (memq (get-buffer bn1) bufobjs)
                                         ;; Place new buffers at the end.
                                         bufobjs))
                             (length (or (memq (get-buffer bn2) bufobjs)
                                         bufobjs))))))



        Stefan

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Leo]

On 2010-10-18 22:54 +0800, Stefan Monnier wrote:
> I was about to install that change when I realized that this is
> fundamentally not the right approach: since some of the buffers may have
> changed name, the new list of matching buffers may be different (some
> buffers that didn't match before may match now and vice-versa).
>
> So iswitchb-make-buflist is more correct.  To deal with the problem of
> ordering, we'll need to combine the two: call iswitchb-make-buflist to
> get the new list of matches, and then use bufobjs to sort the new
> iswitchb-buflist.

My understanding seems to be:

iswitchb-buflist already contains all buffers needed although its order
can be modified by iswitchb-next/prev-match. That modified ordering info
is lost after iswitchb-make-buflist. There may be new matches appearing
due to buffer name changes but this is taken care of automatically by
iswitchb-exhibit.

Leo

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Leo]

Hello Stefan,

On 2010-10-20 11:00 +0800, Leo wrote:
> On 2010-10-18 22:54 +0800, Stefan Monnier wrote:
>> I was about to install that change when I realized that this is
>> fundamentally not the right approach: since some of the buffers may have
>> changed name, the new list of matching buffers may be different (some
>> buffers that didn't match before may match now and vice-versa).
>>
>> So iswitchb-make-buflist is more correct.  To deal with the problem of
>> ordering, we'll need to combine the two: call iswitchb-make-buflist to
>> get the new list of matches, and then use bufobjs to sort the new
>> iswitchb-buflist.
>
> My understanding seems to be:
>
> iswitchb-buflist already contains all buffers needed although its order
> can be modified by iswitchb-next/prev-match. That modified ordering info
> is lost after iswitchb-make-buflist. There may be new matches appearing
> due to buffer name changes but this is taken care of automatically by
> iswitchb-exhibit.
>
> Leo

If the proposed solution to iswitchb is acceptable, a similar one should
be done for ido.

Here is a patch to be applied on top of
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6943.

From 5686c7c723d9bad601d870aff54b7a3d8edce471 Mon Sep 17 00:00:00 2001
Date: Wed, 20 Oct 2010 12:17:53 +0800
Subject: [PATCH] Don't rebuild buffer list in ido-kill-buffer-at-head

Rebuilding buffer list will lose the order of matches seen by the user
in particular when it has been rotated.
---
 lisp/ido.el |   32 ++++++++++++++------------------
 1 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 4a60288..d913069 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -3978,26 +3978,22 @@ If cursor is not at the end of the user input, delete to end of input."
   (if (not (eobp))
       (delete-region (point) (line-end-position))
     (let ((enable-recursive-minibuffers t)
-	  (buf (ido-name (car ido-matches)))
-	  (nextbuf (cadr ido-matches)))
+	  (buf (ido-name (car ido-matches))))
       (cond
        ((get-buffer buf)
-	;; If next match names a buffer use the buffer object; buffer
-	;; name may be changed by packages such as uniquify.
-	(when (and nextbuf (get-buffer nextbuf))
-	  (setq nextbuf (get-buffer nextbuf)))
-	(if (null (kill-buffer buf))
-	    ;; Buffer couldn't be killed.
-	    (setq ido-rescan t)
-	  ;; Else `kill-buffer' succeeds so re-make the buffer list
-	  ;; taking into account packages like uniquify may rename
-	  ;; buffers.
-	  (if (bufferp nextbuf)
-	      (setq nextbuf (buffer-name nextbuf)))
-	  (setq ido-default-item nextbuf
-		ido-text-init ido-text
-		ido-exit 'refresh)
-	  (exit-minibuffer)))
+	;; Note that some packages (eg uniquify.el) may change buffer
+	;; names. So save a list of buffer objects.
+	(let ((bufobjs (mapcar (lambda (name)
+				 (or (get-buffer name) name))
+			       ido-cur-list)))
+	  (if (null (kill-buffer buf))
+	      ;; Buffer couldn't be killed.
+	      (setq ido-rescan t)
+	    (setq ido-cur-list
+		  (delq nil (mapcar (lambda (b) (if (bufferp b)
+						    (buffer-name b)
+						  b))
+				    bufobjs))))))
        ;; Handle virtual buffers
        ((assoc buf ido-virtual-buffers)
 	(setq recentf-list
-- 
1.7.3

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Óscar Fuentes]

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

> If the proposed solution to iswitchb is acceptable, a similar one should
> be done for ido.
>
> Here is a patch to be applied on top of
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6943.
>
> From 5686c7c723d9bad601d870aff54b7a3d8edce471 Mon Sep 17 00:00:00 2001
> Date: Wed, 20 Oct 2010 12:17:53 +0800
> Subject: [PATCH] Don't rebuild buffer list in ido-kill-buffer-at-head

Can you post a version that does not require to previosly introduce
another patch, so we can test *this* proposed solution instead of
something else?

[snip]

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Stefan Monnier]

>> I was about to install that change when I realized that this is
>> fundamentally not the right approach: since some of the buffers may have
>> changed name, the new list of matching buffers may be different (some
>> buffers that didn't match before may match now and vice-versa).
>> 
>> So iswitchb-make-buflist is more correct.  To deal with the problem of
>> ordering, we'll need to combine the two: call iswitchb-make-buflist to
>> get the new list of matches, and then use bufobjs to sort the new
>> iswitchb-buflist.

> My understanding seems to be:

> iswitchb-buflist already contains all buffers needed although its order
> can be modified by iswitchb-next/prev-match. That modified ordering info
> is lost after iswitchb-make-buflist. There may be new matches appearing
> due to buffer name changes but this is taken care of automatically by
> iswitchb-exhibit.

I see, so iswitchb-buflist contains all buffers, not just the ones that
match the current input?
In that case, my concern is indeed a non-issue and your patch
looks fine.  Please install it.


        Stefan

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Leo]

On 2010-10-20 12:53 +0800, Leo wrote:
[...]
>
> If the proposed solution to iswitchb is acceptable, a similar one should
> be done for ido.
[...]

On 2010-10-20 17:19 +0800, Óscar Fuentes wrote:
[...]
> Can you post a version that does not require to previosly introduce
> another patch, so we can test *this* proposed solution instead of
> something else?
>
> [snip]

Due to the way ido set matches i.e. it ranges matches into a few
sections unless ido-rotate is non-nil (see #5724) and the order of
rotated matches is lost after any none rotating commands, please ignore
the patch for ido for now.

Could someone having commit access apply the patch for iswitchb? I have
received a complaint from someone via IRC the other day.

Thanks.

Leo

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Leo]

On 2010-10-21 00:13 +0800, Stefan Monnier wrote:
> I see, so iswitchb-buflist contains all buffers, not just the ones that
> match the current input?

Except those being ignored. iswitchb-matches holds the matches.

> In that case, my concern is indeed a non-issue and your patch looks
> fine. Please install it.

Someone in IRC (whose complaint this bug report tries to address) has
also confirmed the patch fixes the issue for him. Could you or someone
else install the patch? Many thanks.

Leo

bug#7231: 23.3; Don't rebuild buffer list in iswitchb-visit-buffer

[Stefan Monnier]

> Sorry for being too lazy. Please try the following patch.

Thanks, installed in the trunk.


        Stefan