PATCH: Fix IDO interaction with uniquify.el

PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

When `uniquify' is in effect, killing a buffer may cause the renaming of
others.

While switching buffers with ido-mode enabled, the user is presented
with a list of buffer names to choose. From the prompt he can kill the
first buffer list, which is removed from the list, and keeps waiting for
further user interaction. However, IDO does not take into account that
some buffers may have changed names.

This patch rebuilds the list of buffer names used by IDO after a buffer
is killed if some name contained in the list no longer maps to an
existing buffer.

BTW, iswitchb-mode suffers from the same problem.

2010-01-18  Óscar Fuentes  <ofv@wanadoo.es>

	* ido.el (ido-make-buffer-list): If `default' is a nonexistent
          buffer, ignore it, as per the documentation.
          (ido-kill-buffer-internal): New function.
          (ido-kill-buffer-at-head): Use it.
          (ido-visit-buffer): Likewise.

=== modified file 'lisp/ido.el'
--- lisp/ido.el	2010-01-04 05:35:18 +0000
+++ lisp/ido.el	2010-01-18 10:25:26 +0000
@@ -3344,7 +3344,7 @@
     (if ido-temp-list
 	(nconc ido-temp-list ido-current-buffers)
       (setq ido-temp-list ido-current-buffers))
-    (if default
+    (if (and default (buffer-live-p (get-buffer default)))
 	(progn
 	  (setq ido-temp-list
 		(delete default ido-temp-list))
@@ -3830,6 +3830,31 @@
 	      ;;(add-hook 'completion-setup-hook 'completion-setup-function)
 	      (display-completion-list completion-list)))))))
 
+(defun ido-kill-buffer-internal (buf)
+  "Actually kill the buffer and check if it is needed to rebuild
+the list of known buffers."
+  (kill-buffer buf)
+  (if (get-buffer buf)
+      ;; buffer couldn't be killed.
+      (setq ido-rescan t)
+    (let ((next-buf (cadr ido-matches))
+	  (needs-update nil))
+      ;; else buffer was killed so remove name from list.
+      (setq ido-cur-list (delq buf ido-cur-list))
+      ;; Some packages, like uniquify.el, may rename buffers when one
+      ;; is killed, so we need to test this condition to avoid using
+      ;; an outdated list of buffer names. We don't want to always
+      ;; rebuild the list of buffers, as this alters the previous
+      ;; buffer order that the user was seeing on the prompt. However,
+      ;; when we rebuild the list, we try to keep the previous second
+      ;; buffer as the first one.
+      (dolist (b ido-cur-list)
+	(if (not (get-buffer b))
+	    (setq needs-update t)))
+      (when needs-update
+	(setq ido-cur-list (ido-make-buffer-list next-buf))
+	(setq ido-rescan t)))))
+
 ;;; KILL CURRENT BUFFER
 (defun ido-kill-buffer-at-head ()
   "Kill the buffer at the head of `ido-matches'.
@@ -3840,7 +3865,7 @@
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
+	(ido-kill-buffer-internal buf)
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
@@ -3884,7 +3909,7 @@
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-      (kill-buffer buffer))
+      (ido-kill-buffer-internal buffer))
 
      ((eq method 'other-window)
       (if record

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Mon, Jan 18, 2010 at 11:27, Óscar Fuentes <ofv@wanadoo.es> wrote:

> 2010-01-18  Óscar Fuentes  <ofv@wanadoo.es>
>
>        * ido.el (ido-make-buffer-list): If `default' is a nonexistent
>          buffer, ignore it, as per the documentation.
>          (ido-kill-buffer-internal): New function.
>          (ido-kill-buffer-at-head): Use it.
>          (ido-visit-buffer): Likewise.

Nice. A problem, though:

 emacs -Q -f ido-mode
 C-x k <RET>

 => Symbol's value as variable is void: ido-cur-list


    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

Another weirdness:

  cd c:\emacs\trunk
  emacs -Q -l uniquify -f ido-mode --eval "(setq
uniquify-buffer-name-style 'forward)"
  C-x C-f ChangeLog <RET>
  C-x C-f src/ChangeLog <RET>
  C-x b     =>   "buffer: {trunk/ChangeLog | *scratch* | *Messages* |
src/ChangeLog}"
  C-k       => "buffer: {*scratch* | *Messages* |  *Minibuf-1* | ChangeLog}"

" *Minibuf-1*" is unexpected.

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

Hello Juanma.

Thanks for testing. Comments and a new patch follows.

Juanma Barranquero <lekktu@gmail.com> writes:

> Nice. A problem, though:
>
>  emacs -Q -f ido-mode
>  C-x k <RET>
>
>  => Symbol's value as variable is void: ido-cur-list

This is solved by using

(defvar ido-cur-list nil)

instead of

(defvar ido-cur-list)

Hope it's acceptable.

> Another weirdness:
>
>   cd c:\emacs\trunk
>   emacs -Q -l uniquify -f ido-mode --eval "(setq
> uniquify-buffer-name-style 'forward)"
>   C-x C-f ChangeLog <RET>
>   C-x C-f src/ChangeLog <RET>
>   C-x b     =>   "buffer: {trunk/ChangeLog | *scratch* | *Messages* |
> src/ChangeLog}"
>   C-k       => "buffer: {*scratch* | *Messages* |  *Minibuf-1* | ChangeLog}"
>
> " *Minibuf-1*" is unexpected.

This is solved by explicitly ignoring minibuffers while gathering the
list of buffers.

2010-01-18  Óscar Fuentes  <ofv@wanadoo.es>

	* ido.el (ido-cur-list): Initialize as nil. Remove obsolete
          information from its commentary.
          (ido-choice-list): Initialize as nil.
          (ido-get-bufname): Reject minibuffers.
          (ido-make-buffer-list): If "default" is a nonexistent
          buffer, ignore it, as per the docstring.
          (ido-kill-buffer-internal): New function.
          (ido-kill-buffer-at-head): Use it.
          (ido-visit-buffer): Likewise.

=== modified file 'lisp/ido.el'
--- lisp/ido.el	2010-01-13 08:35:10 +0000
+++ lisp/ido.el	2010-01-18 14:06:20 +0000
@@ -1042,11 +1042,11 @@
 ;; Stores the current list of items that will be searched through.
 ;; The list is ordered, so that the most interesting item comes first,
 ;; although by default, the files visible in the current frame are put
-;; at the end of the list.  Created by `ido-make-item-list'.
-(defvar ido-cur-list)
+;; at the end of the list.
+(defvar ido-cur-list nil)
 
 ;; Stores the choice list for ido-completing-read
-(defvar ido-choice-list)
+(defvar ido-choice-list nil)
 
 ;; Stores the list of items which are ignored when building
 ;; `ido-cur-list'.  It is in no specific order.
@@ -3344,7 +3344,7 @@
     (if ido-temp-list
 	(nconc ido-temp-list ido-current-buffers)
       (setq ido-temp-list ido-current-buffers))
-    (if default
+    (if (and default (buffer-live-p (get-buffer default)))
 	(progn
 	  (setq ido-temp-list
 		(delete default ido-temp-list))
@@ -3590,6 +3590,7 @@
   ;; Used by `ido-get-buffers-in-frames' to walk through all windows
   (let ((buf (buffer-name (window-buffer win))))
 	(unless (or (member buf ido-bufs-in-frame)
+		    (minibufferp buf)
 		    (member buf ido-ignore-item-temp-list))
 	  ;; Only add buf if it is not already in list.
 	  ;; This prevents same buf in two different windows being
@@ -3830,6 +3831,32 @@
 	      ;;(add-hook 'completion-setup-hook 'completion-setup-function)
 	      (display-completion-list completion-list)))))))
 
+(defun ido-kill-buffer-internal (buf)
+  "Actually kill the buffer and check if it is needed to rebuild
+the list of known buffers."
+  (kill-buffer buf)
+  (if (get-buffer buf)
+      ;; buffer couldn't be killed.
+      (setq ido-rescan t)
+    (let ((next-buf (cadr ido-matches))
+	  (needs-update nil))
+      ;; else buffer was killed so remove name from list.
+      (setq ido-cur-list (delq buf ido-cur-list))
+      ;; Some packages, like uniquify.el, may rename buffers when one
+      ;; is killed, so we need to test this condition to avoid using
+      ;; an outdated list of buffer names. We don't want to always
+      ;; rebuild the list of buffers, as this alters the previous
+      ;; buffer order that the user was seeing on the prompt. However,
+      ;; when we rebuild the list, we try to keep the previous second
+      ;; buffer as the first one.
+      (dolist (b ido-cur-list)
+	(if (not (get-buffer b))
+	    (setq needs-update t)))
+      (when needs-update
+	(setq ido-cur-list (ido-make-buffer-list next-buf))
+	(setq ido-rescan t)
+	(ido-set-matches)))))
+
 ;;; KILL CURRENT BUFFER
 (defun ido-kill-buffer-at-head ()
   "Kill the buffer at the head of `ido-matches'.
@@ -3840,7 +3867,7 @@
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
+	(ido-kill-buffer-internal buf)
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
@@ -3884,7 +3911,7 @@
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-      (kill-buffer buffer))
+      (ido-kill-buffer-internal buffer))
 
      ((eq method 'other-window)
       (if record

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Mon, Jan 18, 2010 at 15:13, Óscar Fuentes <ofv@wanadoo.es> wrote:

> Comments and a new patch follows.

Yes, it works now.

BTW, why

> +  (kill-buffer buf)
> +  (if (get-buffer buf)

`kill-buffer' returns t iff it was able to kill the buffer, so

  (if (not (kill-buffer buf))

should be enough...

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

Juanma Barranquero <lekktu@gmail.com> writes:

> BTW, why
>
>> +  (kill-buffer buf)
>> +  (if (get-buffer buf)
>
> `kill-buffer' returns t iff it was able to kill the buffer, so
>
>   (if (not (kill-buffer buf))
>
> should be enough...

This patch incorporates your suggestion and removes an unneeded call to
ido-set-matches.

2010-01-18  Óscar Fuentes  <ofv@wanadoo.es>

	* ido.el (ido-cur-list): Initialize as nil. Remove obsolete
          information from commentary.
          (ido-choice-list): Initialize as nil.
          (ido-get-bufname): Reject minibuffers.
          (ido-make-buffer-list): If "default" is a nonexistent
          buffer, ignore it, as per the docstring.
          (ido-kill-buffer-internal): New function.
          (ido-kill-buffer-at-head): Use it.
          (ido-visit-buffer): Likewise.

=== modified file 'lisp/ido.el'
--- lisp/ido.el	2010-01-13 08:35:10 +0000
+++ lisp/ido.el	2010-01-18 14:35:24 +0000
@@ -1042,11 +1042,11 @@
 ;; Stores the current list of items that will be searched through.
 ;; The list is ordered, so that the most interesting item comes first,
 ;; although by default, the files visible in the current frame are put
-;; at the end of the list.  Created by `ido-make-item-list'.
-(defvar ido-cur-list)
+;; at the end of the list.
+(defvar ido-cur-list nil)
 
 ;; Stores the choice list for ido-completing-read
-(defvar ido-choice-list)
+(defvar ido-choice-list nil)
 
 ;; Stores the list of items which are ignored when building
 ;; `ido-cur-list'.  It is in no specific order.
@@ -3344,7 +3344,7 @@
     (if ido-temp-list
 	(nconc ido-temp-list ido-current-buffers)
       (setq ido-temp-list ido-current-buffers))
-    (if default
+    (if (and default (buffer-live-p (get-buffer default)))
 	(progn
 	  (setq ido-temp-list
 		(delete default ido-temp-list))
@@ -3590,6 +3590,7 @@
   ;; Used by `ido-get-buffers-in-frames' to walk through all windows
   (let ((buf (buffer-name (window-buffer win))))
 	(unless (or (member buf ido-bufs-in-frame)
+		    (minibufferp buf)
 		    (member buf ido-ignore-item-temp-list))
 	  ;; Only add buf if it is not already in list.
 	  ;; This prevents same buf in two different windows being
@@ -3830,6 +3831,30 @@
 	      ;;(add-hook 'completion-setup-hook 'completion-setup-function)
 	      (display-completion-list completion-list)))))))
 
+(defun ido-kill-buffer-internal (buf)
+  "Actually kill the buffer and check if it is needed to rebuild
+the list of known buffers."
+  (if (not (kill-buffer buf))
+      ;; buffer couldn't be killed.
+      (setq ido-rescan t)
+    (let ((next-buf (cadr ido-matches))
+	  (needs-update nil))
+      ;; else buffer was killed so remove name from list.
+      (setq ido-cur-list (delq buf ido-cur-list))
+      ;; Some packages, like uniquify.el, may rename buffers when one
+      ;; is killed, so we need to test this condition to avoid using
+      ;; an outdated list of buffer names. We don't want to always
+      ;; rebuild the list of buffers, as this alters the previous
+      ;; buffer order that the user was seeing on the prompt. However,
+      ;; when we rebuild the list, we try to keep the previous second
+      ;; buffer as the first one.
+      (dolist (b ido-cur-list)
+	(if (not (get-buffer b))
+	    (setq needs-update t)))
+      (when needs-update
+	(setq ido-cur-list (ido-make-buffer-list next-buf))
+	(setq ido-rescan t)))))
+
 ;;; KILL CURRENT BUFFER
 (defun ido-kill-buffer-at-head ()
   "Kill the buffer at the head of `ido-matches'.
@@ -3840,7 +3865,7 @@
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
+	(ido-kill-buffer-internal buf)
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
@@ -3884,7 +3909,7 @@
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-      (kill-buffer buffer))
+      (ido-kill-buffer-internal buffer))
 
      ((eq method 'other-window)
       (if record

Re: PATCH: Fix IDO interaction with uniquify.el

[Chong Yidong]

Óscar Fuentes <ofv@wanadoo.es> writes:

> +(defun ido-kill-buffer-internal (buf)
> +  "Actually kill the buffer and check if it is needed to rebuild
> +the list of known buffers."

The docstring does not follow conventions.  The first line should be a
single sentence.  Something like this should suffice

"Kill buffer BUF and and rebuild ido's buffer list if needed."

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

Chong Yidong <cyd@stupidchicken.com> writes:

> Óscar Fuentes <ofv@wanadoo.es> writes:
>
>> +(defun ido-kill-buffer-internal (buf)
>> +  "Actually kill the buffer and check if it is needed to rebuild
>> +the list of known buffers."
>
> The docstring does not follow conventions.  The first line should be a
> single sentence.  Something like this should suffice
>
> "Kill buffer BUF and and rebuild ido's buffer list if needed."

Okay. Updated docstring and incorporated a style improvement suggested
by Juanma.

2010-01-18  Óscar Fuentes  <ofv@wanadoo.es>

	* ido.el (ido-cur-list): Initialize as nil. Remove obsolete
          information from commentary.
          (ido-choice-list): Initialize as nil.
          (ido-get-bufname): Reject minibuffers.
          (ido-make-buffer-list): If "default" is a nonexistent
          buffer, ignore it, as per the docstring.
          (ido-kill-buffer-internal): New function.
          (ido-kill-buffer-at-head): Use it.
          (ido-visit-buffer): Likewise.

=== modified file 'lisp/ido.el'
--- lisp/ido.el	2010-01-13 08:35:10 +0000
+++ lisp/ido.el	2010-01-18 17:32:44 +0000
@@ -1042,11 +1042,11 @@
 ;; Stores the current list of items that will be searched through.
 ;; The list is ordered, so that the most interesting item comes first,
 ;; although by default, the files visible in the current frame are put
-;; at the end of the list.  Created by `ido-make-item-list'.
-(defvar ido-cur-list)
+;; at the end of the list.
+(defvar ido-cur-list nil)
 
 ;; Stores the choice list for ido-completing-read
-(defvar ido-choice-list)
+(defvar ido-choice-list nil)
 
 ;; Stores the list of items which are ignored when building
 ;; `ido-cur-list'.  It is in no specific order.
@@ -3344,7 +3344,7 @@
     (if ido-temp-list
 	(nconc ido-temp-list ido-current-buffers)
       (setq ido-temp-list ido-current-buffers))
-    (if default
+    (if (and default (buffer-live-p (get-buffer default)))
 	(progn
 	  (setq ido-temp-list
 		(delete default ido-temp-list))
@@ -3590,6 +3590,7 @@
   ;; Used by `ido-get-buffers-in-frames' to walk through all windows
   (let ((buf (buffer-name (window-buffer win))))
 	(unless (or (member buf ido-bufs-in-frame)
+		    (minibufferp buf)
 		    (member buf ido-ignore-item-temp-list))
 	  ;; Only add buf if it is not already in list.
 	  ;; This prevents same buf in two different windows being
@@ -3830,6 +3831,29 @@
 	      ;;(add-hook 'completion-setup-hook 'completion-setup-function)
 	      (display-completion-list completion-list)))))))
 
+(defun ido-kill-buffer-internal (buf)
+  "Kill buffer BUF and rebuild ido's buffer list if needed."
+  (if (not (kill-buffer buf))
+      ;; buffer couldn't be killed.
+      (setq ido-rescan t)
+    (let ((next-buf (cadr ido-matches))
+	  (needs-update nil))
+      ;; else buffer was killed so remove name from list.
+      (setq ido-cur-list (delq buf ido-cur-list))
+      ;; Some packages, like uniquify.el, may rename buffers when one
+      ;; is killed, so we need to test this condition to avoid using
+      ;; an outdated list of buffer names. We don't want to always
+      ;; rebuild the list of buffers, as this alters the previous
+      ;; buffer order that the user was seeing on the prompt. However,
+      ;; when we rebuild the list, we try to keep the previous second
+      ;; buffer as the first one.
+      (catch 'update
+	(dolist (b ido-cur-list)
+	  (unless (get-buffer b)
+	    (setq ido-cur-list (ido-make-buffer-list next-buf))
+	    (setq ido-rescan t)
+	    (throw 'update nil)))))))
+
 ;;; KILL CURRENT BUFFER
 (defun ido-kill-buffer-at-head ()
   "Kill the buffer at the head of `ido-matches'.
@@ -3840,7 +3864,7 @@
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
+	(ido-kill-buffer-internal buf)
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
@@ -3884,7 +3908,7 @@
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-      (kill-buffer buffer))
+      (ido-kill-buffer-internal buffer))
 
      ((eq method 'other-window)
       (if record

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

Óscar Fuentes <ofv@wanadoo.es> writes:

This is the definitive one.

... or so I hope.

Removed a `let' that was unnecessary after the style improvement.


2010-01-18  Óscar Fuentes  <ofv@wanadoo.es>

	* ido.el (ido-cur-list): Initialize as nil. Remove obsolete
          information from commentary.
          (ido-choice-list): Initialize as nil.
          (ido-get-bufname): Reject minibuffers.
          (ido-make-buffer-list): If "default" is a nonexistent
          buffer, ignore it, as per the docstring.
          (ido-kill-buffer-internal): New function.
          (ido-kill-buffer-at-head): Use it.
          (ido-visit-buffer): Likewise.


=== modified file 'lisp/ido.el'
--- lisp/ido.el	2010-01-13 08:35:10 +0000
+++ lisp/ido.el	2010-01-18 17:45:50 +0000
@@ -1042,11 +1042,11 @@
 ;; Stores the current list of items that will be searched through.
 ;; The list is ordered, so that the most interesting item comes first,
 ;; although by default, the files visible in the current frame are put
-;; at the end of the list.  Created by `ido-make-item-list'.
-(defvar ido-cur-list)
+;; at the end of the list.
+(defvar ido-cur-list nil)
 
 ;; Stores the choice list for ido-completing-read
-(defvar ido-choice-list)
+(defvar ido-choice-list nil)
 
 ;; Stores the list of items which are ignored when building
 ;; `ido-cur-list'.  It is in no specific order.
@@ -3344,7 +3344,7 @@
     (if ido-temp-list
 	(nconc ido-temp-list ido-current-buffers)
       (setq ido-temp-list ido-current-buffers))
-    (if default
+    (if (and default (buffer-live-p (get-buffer default)))
 	(progn
 	  (setq ido-temp-list
 		(delete default ido-temp-list))
@@ -3590,6 +3590,7 @@
   ;; Used by `ido-get-buffers-in-frames' to walk through all windows
   (let ((buf (buffer-name (window-buffer win))))
 	(unless (or (member buf ido-bufs-in-frame)
+		    (minibufferp buf)
 		    (member buf ido-ignore-item-temp-list))
 	  ;; Only add buf if it is not already in list.
 	  ;; This prevents same buf in two different windows being
@@ -3830,6 +3831,28 @@
 	      ;;(add-hook 'completion-setup-hook 'completion-setup-function)
 	      (display-completion-list completion-list)))))))
 
+(defun ido-kill-buffer-internal (buf)
+  "Kill buffer BUF and rebuild ido's buffer list if needed."
+  (if (not (kill-buffer buf))
+      ;; buffer couldn't be killed.
+      (setq ido-rescan t)
+    (progn
+      ;; else buffer was killed so remove name from list.
+      (setq ido-cur-list (delq buf ido-cur-list))
+      ;; Some packages, like uniquify.el, may rename buffers when one
+      ;; is killed, so we need to test this condition to avoid using
+      ;; an outdated list of buffer names. We don't want to always
+      ;; rebuild the list of buffers, as this alters the previous
+      ;; buffer order that the user was seeing on the prompt. However,
+      ;; when we rebuild the list, we try to keep the previous second
+      ;; buffer as the first one.
+      (catch 'update
+	(dolist (b ido-cur-list)
+	  (unless (get-buffer b)
+	    (setq ido-cur-list (ido-make-buffer-list (cadr ido-matches)))
+	    (setq ido-rescan t)
+	    (throw 'update nil)))))))
+
 ;;; KILL CURRENT BUFFER
 (defun ido-kill-buffer-at-head ()
   "Kill the buffer at the head of `ido-matches'.
@@ -3840,7 +3863,7 @@
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
+	(ido-kill-buffer-internal buf)
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
@@ -3884,7 +3907,7 @@
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-      (kill-buffer buffer))
+      (ido-kill-buffer-internal buffer))
 
      ((eq method 'other-window)
       (if record

Re: PATCH: Fix IDO interaction with uniquify.el

[Chong Yidong]

Óscar Fuentes <ofv@wanadoo.es> writes:

> This is the definitive one.
>
> ... or so I hope.
>
> Removed a `let' that was unnecessary after the style improvement.

Looks OK to me.  I don't use ido, however, so could someone who does
(and has commit access) check that it behaves correctly, and if so
commit it?

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Mon, Jan 18, 2010 at 19:06, Chong Yidong <cyd@stupidchicken.com> wrote:

> Looks OK to me.  I don't use ido, however, so could someone who does
> (and has commit access) check that it behaves correctly, and if so
> commit it?

I use ido. In my not-that-extensive testing, it works as expected and
fixes the bug. I'll commit it.

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

On 2010-01-18 19:17 +0000, Juanma Barranquero wrote:
> On Mon, Jan 18, 2010 at 19:06, Chong Yidong <cyd@stupidchicken.com> wrote:
>
>> Looks OK to me.  I don't use ido, however, so could someone who does
>> (and has commit access) check that it behaves correctly, and if so
>> commit it?
>
> I use ido. In my not-that-extensive testing, it works as expected and
> fixes the bug. I'll commit it.
>
>     Juanma

I confirm the original bug report. I propose here a different fix for
both ido and iswitch. It is much simpler and more efficient.
(buffer-list) can be a large list given the new feature virtual buffers
in both iswitch and ido modes.

Leo

From 4faddfd77236de485d77eece0eb7b655e85d9cdf Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Wed, 5 May 2010 09:19:59 +0100
Subject: [PATCH] Stop uniquify from interfering with iswitch and ido when killing buffers

---
 lisp/ido.el      |    3 ++-
 lisp/iswitchb.el |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 7cdeb70..9d0cb40 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -3965,7 +3965,8 @@ If cursor is not at the end of the user input, delete to end of input."
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
+	;; stop uniquify from renaming buffers
+	(let (uniquify-buffer-name-style) (kill-buffer buf))
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
diff --git a/lisp/iswitchb.el b/lisp/iswitchb.el
index ea4b00d..333eeee 100644
--- a/lisp/iswitchb.el
+++ b/lisp/iswitchb.el
@@ -1034,7 +1034,8 @@ Return the modified list with the last element prepended to it."
     ;; check to see if buf is non-nil.
     (if buf
 	(progn
-	  (kill-buffer buf)
+	  ;; stop uniquify from renaming buffers
+	  (let (uniquify-buffer-name-style) (kill-buffer buf))
 
 	  ;; Check if buffer exists.  XEmacs gnuserv.el makes alias
 	  ;; for kill-buffer which does not return t if buffer is
-- 
1.7.0.4

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Wed, May 5, 2010 at 10:27, Leo <sdl.web@gmail.com> wrote:

> I confirm the original bug report. I propose here a different fix for
> both ido and iswitch. It is much simpler and more efficient.

On the minus side, your patch:

  - is less generic; Óscar's will work for other packages that do
dynamic renaming of buffers (yes, there are none on the Emacs
distribution, but there's a lot of elisp code out there).
  - makes ido depend on a feature of uniquify, which it didn't before.

> (buffer-list) can be a large list given the new feature virtual buffers
> in both iswitch and ido modes.

Has that been a problem for you? It should only be if you have a
really long list of buffers *and* use ido-kill-buffer a lot on the
same ido-switch-buffer invocation...

IMHO the virtual buffers feature is still a bit unfinished. For
example, it doesn't support multiple "virtual buffers" for files with
the same name on different directories. There was talk of using
uniquify to fix that, but again, my feeling is that tying ido to
uniquify is unwise; the virtual buffers implementation (on ido) should
fix its own problems in a more generic way.

Just my 0,02€

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

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

On 5 May 2010 10:56, Juanma Barranquero <lekktu@gmail.com> wrote:
> [......]

I take the point. I have included a patch that fix the bug in a more
general but still cleaner way. Please let me know if it works for you
in both ido and iswitchb too. Thanks.

> IMHO the virtual buffers feature is still a bit unfinished. For
> example, it doesn't support multiple "virtual buffers" for files with
> the same name on different directories. There was talk of using
> uniquify to fix that, but again, my feeling is that tying ido to
> uniquify is unwise; the virtual buffers implementation (on ido) should
> fix its own problems in a more generic way.

This is put on hold until FSF received my paper. As soon as that
happens John will push patches from his repo, one of which will also
enable C-k to kill virtual buffers.

> Just my 0,02€
>
>    Juanma

Leo

[-- Attachment #2: 0001-Fix-a-bug-in-both-ido-and-switchb-related-to-killing.patch --]
[-- Type: application/octet-stream, Size: 1932 bytes --]

From 908978b51bb0a5edf0af01b42222887ac2ce12a6 Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Wed, 5 May 2010 13:33:13 +0100
Subject: [PATCH] Fix a bug in both ido and switchb related to killing buffers

by rebuilding buffer list since packages like uniquify may rename
buffers.
---
 lisp/ido.el      |   11 ++++++-----
 lisp/iswitchb.el |    5 +++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 7cdeb70..298eca7 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -3965,13 +3965,14 @@ If cursor is not at the end of the user input, delete to end of input."
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
-	;; Check if buffer still exists.
-	(if (get-buffer buf)
+	(if (null (kill-buffer buf))
 	    ;; buffer couldn't be killed.
 	    (setq ido-rescan t)
-	  ;; else buffer was killed so remove name from list.
-	  (setq ido-cur-list (delq buf ido-cur-list)))))))
+	  ;; else re-generate the buffer list taking into account
+	  ;; packages like uniquify may rename buffers
+	  (setq ido-text-init ido-text)
+	  (setq ido-exit 'refresh)
+	  (exit-minibuffer))))))
 
 ;;; DELETE CURRENT FILE
 (defun ido-delete-file-at-head ()
diff --git a/lisp/iswitchb.el b/lisp/iswitchb.el
index ea4b00d..4e939c5 100644
--- a/lisp/iswitchb.el
+++ b/lisp/iswitchb.el
@@ -1042,8 +1042,9 @@ 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 buffer was killed so remove name from list.
-	    (setq iswitchb-buflist  (delq buf iswitchb-buflist)))))))
+	    ;; else re-generate the buffer list taking into account
+	    ;; packages like uniquify may rename buffers
+	    (iswitchb-make-buflist iswitchb-default))))))
 
 ;;; VISIT CHOSEN BUFFER
 (defun iswitchb-visit-buffer (buffer)
-- 
1.7.0.4

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Wed, May 5, 2010 at 14:40, Leo <sdl.web@googlemail.com> wrote:

> I take the point. I have included a patch that fix the bug in a more
> general but still cleaner way. Please let me know if it works for you
> in both ido and iswitchb too. Thanks.

The ido part is not against the trunk. Could you please resend it?

As for iswitchb, I don't use it; that said, in my very informal and
brief test, yeah, it works as expected.

> This is put on hold until FSF received my paper. As soon as that
> happens John will push patches from his repo, one of which will also
> enable C-k to kill virtual buffers.

What does exactly mean "kill virtual buffers"? If you mean removing
them from the buffer list, are you going to maintain a separate list,
instead of regenerating it from recentf-list anew for each
ido-switch-buffer, or are you going to modify recentf-list?

As for the duplicate entries, have you implemented it without using
uniquify? That does not seem too hard, and it's definitely cleaner.

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

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

On 5 May 2010 17:47, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Wed, May 5, 2010 at 14:40, Leo <sdl.web@googlemail.com> wrote:
>
>> I take the point. I have included a patch that fix the bug in a more
>> general but still cleaner way. Please let me know if it works for you
>> in both ido and iswitchb too. Thanks.
>
> The ido part is not against the trunk. Could you please resend it?
>
> As for iswitchb, I don't use it; that said, in my very informal and
> brief test, yeah, it works as expected.

As attached with some simplification. Let me know if the ido part works. Thanks.

Leo

[-- Attachment #2: 0001-Back-out-the-patch-from-scar-Fuentes.patch --]
[-- Type: application/octet-stream, Size: 4075 bytes --]

From 32cc0c3d93ac9b73a0a12a538c054292dd8cced8 Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Wed, 5 May 2010 18:02:26 +0100
Subject: [PATCH] =?utf-8?q?Back=20out=20the=20patch=20from=20=C3=93scar=20Fuentes?=
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

---
 lisp/ido.el |   38 ++++++++------------------------------
 1 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index f75f029..d921dfa 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -1070,11 +1070,11 @@ Only used if `ido-use-virtual-buffers' is non-nil.")
 ;; Stores the current list of items that will be searched through.
 ;; The list is ordered, so that the most interesting item comes first,
 ;; although by default, the files visible in the current frame are put
-;; at the end of the list.
-(defvar ido-cur-list nil)
+;; at the end of the list.  Created by `ido-make-item-list'.
+(defvar ido-cur-list)
 
 ;; Stores the choice list for ido-completing-read
-(defvar ido-choice-list nil)
+(defvar ido-choice-list)
 
 ;; Stores the list of items which are ignored when building
 ;; `ido-cur-list'.  It is in no specific order.
@@ -3400,9 +3400,9 @@ for first matching file."
     (if ido-temp-list
 	(nconc ido-temp-list ido-current-buffers)
       (setq ido-temp-list ido-current-buffers))
-    (when (and default (buffer-live-p (get-buffer default)))
-      (setq ido-temp-list
-	    (cons default (delete default ido-temp-list))))
+    (if default
+        (setq ido-temp-list
+              (cons default (delete default ido-temp-list))))
     (if ido-use-virtual-buffers
 	(ido-add-virtual-buffers-to-list))
     (run-hooks 'ido-make-buffer-list-hook)
@@ -3672,7 +3672,6 @@ This is to make them appear as if they were \"virtual buffers\"."
   ;; Used by `ido-get-buffers-in-frames' to walk through all windows
   (let ((buf (buffer-name (window-buffer win))))
 	(unless (or (member buf ido-bufs-in-frame)
-		    (minibufferp buf)
 		    (member buf ido-ignore-item-temp-list))
 	  ;; Only add buf if it is not already in list.
 	  ;; This prevents same buf in two different windows being
@@ -3913,27 +3912,6 @@ This is to make them appear as if they were \"virtual buffers\"."
 	      ;;(add-hook 'completion-setup-hook 'completion-setup-function)
 	      (display-completion-list completion-list)))))))
 
-(defun ido-kill-buffer-internal (buf)
-  "Kill buffer BUF and rebuild ido's buffer list if needed."
-  (if (not (kill-buffer buf))
-      ;; buffer couldn't be killed.
-      (setq ido-rescan t)
-    ;; else buffer was killed so remove name from list.
-    (setq ido-cur-list (delq buf ido-cur-list))
-    ;; Some packages, like uniquify.el, may rename buffers when one
-    ;; is killed, so we need to test this condition to avoid using
-    ;; an outdated list of buffer names. We don't want to always
-    ;; rebuild the list of buffers, as this alters the previous
-    ;; buffer order that the user was seeing on the prompt. However,
-    ;; when we rebuild the list, we try to keep the previous second
-    ;; buffer as the first one.
-    (catch 'update
-      (dolist (b ido-cur-list)
-	(unless (get-buffer b)
-	  (setq ido-cur-list (ido-make-buffer-list (cadr ido-matches)))
-	  (setq ido-rescan t)
-	  (throw 'update nil))))))
-
 ;;; KILL CURRENT BUFFER
 (defun ido-kill-buffer-at-head ()
   "Kill the buffer at the head of `ido-matches'.
@@ -3944,7 +3922,7 @@ If cursor is not at the end of the user input, delete to end of input."
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(ido-kill-buffer-internal buf)
+	(kill-buffer buf)
 	;; Check if buffer still exists.
 	(if (get-buffer buf)
 	    ;; buffer couldn't be killed.
@@ -3988,7 +3966,7 @@ Record command in `command-history' if optional RECORD is non-nil."
      ((eq method 'kill)
       (if record
 	  (ido-record-command 'kill-buffer buffer))
-      (ido-kill-buffer-internal buffer))
+      (kill-buffer buffer))
 
      ((eq method 'other-window)
       (if record
-- 
1.6.0.2


[-- Attachment #3: 0002-Fix-a-bug-in-both-ido-and-switchb-related-to-killing.patch --]
[-- Type: application/octet-stream, Size: 1986 bytes --]

From 4243e764baddfd57f3c1f90a3f99e976d3088972 Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Wed, 5 May 2010 18:03:47 +0100
Subject: [PATCH] Fix a bug in both ido and switchb related to killing buffers

by rebuilding buffer list since packages like uniquify may rename
buffers.
---
 lisp/ido.el      |   12 +++++-------
 lisp/iswitchb.el |    6 ++++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index d921dfa..34192c0 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -3922,13 +3922,11 @@ If cursor is not at the end of the user input, delete to end of input."
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
-	;; Check if buffer still exists.
-	(if (get-buffer buf)
-	    ;; buffer couldn't be killed.
-	    (setq ido-rescan t)
-	  ;; else buffer was killed so remove name from list.
-	  (setq ido-cur-list (delq buf ido-cur-list)))))))
+	(if (kill-buffer buf)
+	    ;; `kill-buffer' succeeds so re-make the buffer list taking
+	    ;; into account packages like uniquify may rename buffers.
+	    (setq ido-cur-list (ido-make-buffer-list (cadr ido-matches))))
+	(setq ido-rescan t)))))
 
 ;;; DELETE CURRENT FILE
 (defun ido-delete-file-at-head ()
diff --git a/lisp/iswitchb.el b/lisp/iswitchb.el
index ea4b00d..2509e9f 100644
--- a/lisp/iswitchb.el
+++ b/lisp/iswitchb.el
@@ -1042,8 +1042,10 @@ 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 buffer was killed so remove name from list.
-	    (setq iswitchb-buflist  (delq buf iswitchb-buflist)))))))
+	    ;; else `kill-buffer' succeeds so re-make the buffer list
+	    ;; taking into account packages like uniquify may rename
+	    ;; buffers
+	    (iswitchb-make-buflist iswitchb-default))))))
 
 ;;; VISIT CHOSEN BUFFER
 (defun iswitchb-visit-buffer (buffer)
-- 
1.6.0.2

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

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

> As attached with some simplification. Let me know if the ido part works. Thanks.
>
> Leo

The simplification was uncalled for and it didn't work. I had to use a
remote machine to generate the patches against trunk and must have
some mixups during testing.

Anyway, here are the correct second patch as proposed. Please try it
instead sorry for the inconvenience.

Leo

[-- Attachment #2: 0002-Fix-a-bug-in-both-ido-and-switchb-related-to-killing.patch --]
[-- Type: application/octet-stream, Size: 1997 bytes --]

From 808d3d2448e61449846b0b9426cf1706741f7eae Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Wed, 5 May 2010 18:03:47 +0100
Subject: [PATCH] Fix a bug in both ido and switchb related to killing buffers

by rebuilding buffer list since packages like uniquify may rename
buffers.
---
 lisp/ido.el      |   12 +++++++-----
 lisp/iswitchb.el |    6 ++++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index d921dfa..bbf7985 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -3922,13 +3922,15 @@ If cursor is not at the end of the user input, delete to end of input."
     (let ((enable-recursive-minibuffers t)
 	  (buf (ido-name (car ido-matches))))
       (when buf
-	(kill-buffer buf)
-	;; Check if buffer still exists.
-	(if (get-buffer buf)
+	(if (null (kill-buffer buf))
 	    ;; buffer couldn't be killed.
 	    (setq ido-rescan t)
-	  ;; else buffer was killed so remove name from list.
-	  (setq ido-cur-list (delq buf ido-cur-list)))))))
+	  ;; else `kill-buffer' succeeds so re-make the buffer list
+	  ;; taking into account packages like uniquify may rename
+	  ;; buffers.
+	  (setq ido-text-init ido-text)
+	  (setq ido-exit 'refresh)
+	  (exit-minibuffer))))))
 
 ;;; DELETE CURRENT FILE
 (defun ido-delete-file-at-head ()
diff --git a/lisp/iswitchb.el b/lisp/iswitchb.el
index ea4b00d..2509e9f 100644
--- a/lisp/iswitchb.el
+++ b/lisp/iswitchb.el
@@ -1042,8 +1042,10 @@ 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 buffer was killed so remove name from list.
-	    (setq iswitchb-buflist  (delq buf iswitchb-buflist)))))))
+	    ;; else `kill-buffer' succeeds so re-make the buffer list
+	    ;; taking into account packages like uniquify may rename
+	    ;; buffers
+	    (iswitchb-make-buflist iswitchb-default))))))
 
 ;;; VISIT CHOSEN BUFFER
 (defun iswitchb-visit-buffer (buffer)
-- 
1.6.0.2

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Wed, May 5, 2010 at 21:12, Leo <sdl.web@googlemail.com> wrote:

> The simplification was uncalled for and it didn't work. I had to use a
> remote machine to generate the patches against trunk and must have
> some mixups during testing.

It still doesn't seem against the trunk. Your patch has:

       (when buf
-	(kill-buffer buf)
-	;; Check if buffer still exists.
-	(if (get-buffer buf)
+	(if (null (kill-buffer buf))

but the code on trunk/lisp/ido.el is

      (when buf
	(ido-kill-buffer-internal buf)
	;; Check if buffer still exists.
	(if (get-buffer buf)

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Stefan Monnier]

> As attached with some simplification. Let me know if the ido part
>  works. Thanks.

I still don't know what it is trying to fix.


        Stefan

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

> What does exactly mean "kill virtual buffers"? If you mean removing
> them from the buffer list, are you going to maintain a separate list,
> instead of regenerating it from recentf-list anew for each
> ido-switch-buffer, or are you going to modify recentf-list?

'kill virtual buffers' means removing them from recentf-list i.e. they
cease to be virtual buffers. So the latter.

> As for the duplicate entries, have you implemented it without using
> uniquify? That does not seem too hard, and it's definitely cleaner.

John and I have discussed whether uniquify should be used. He wants
the virtual buffer name to follow the same style as that from
uniquify. But in order to use unquify, some changes need to be made
since at the moment uniquify is buffer oriented and virtual buffers
are not buffers. Secondly to uniquify completely, the full list of
files with same base name must be known and this adds complexity. I
locally have a patch that completely handles duplicate basenames
(about 40 lines of elisp without using caching) without using uniquify
so I have some idea about the complexity.

In the end we decide temporarily just adding some number
(customisable) of parent directory. In practice one level of parent
directory already significantly removes the chance of a file in
recentf-list being ignored.

>    Juanma

Leo

Re: PATCH: Fix IDO interaction with uniquify.el

[Juanma Barranquero]

On Wed, May 5, 2010 at 19:56, Leo <sdl.web@googlemail.com> wrote:

> 'kill virtual buffers' means removing them from recentf-list i.e. they
> cease to be virtual buffers. So the latter.

I don't like that. Virtual buffers and recentf are different user
facilities, and the fact that virtual buffers *use* recentf is just to
simplify its implementation, IMO. Killing a virtual buffer on
ido-switch-buffer and having the file list on File / Open Recent
change seems uncalled for.

> I locally have a patch that completely handles duplicate basenames
> (about 40 lines of elisp without using caching) without using uniquify

FWIW, that's my preference. I use uniquify, but what you show in the
modeline and what you do prefer on a completion list isn't necessarily
the same thing.

> In the end we decide temporarily just adding some number
> (customisable) of parent directory. In practice one level of parent
> directory already significantly removes the chance of a file in
> recentf-list being ignored.

I don't think so. If you're editing C:/repo/trunk/lisp/bs.el and
C:/repo/emacs-23/lisp/bs.el, you'd need at least two. Why did you
choose this instead of your 40-line complete fix?

    Juanma

Re: PATCH: Fix IDO interaction with uniquify.el

[Stefan Monnier]

>> I confirm the original bug report. I propose here a different fix for
>> both ido and iswitch. It is much simpler and more efficient.
> On the minus side, your patch:
>   - is less generic; Óscar's will work for other packages that do
> dynamic renaming of buffers (yes, there are none on the Emacs
> distribution, but there's a lot of elisp code out there).
>   - makes ido depend on a feature of uniquify, which it didn't before.

Even more problematic: it changes the behavior of uniquify.
Can someone install Oscar's patch?

BTW, I don't understand why his patch changes (defvar ido-cur-list)
and (defvar ido-choice-list) into (defvar ido-cur-list nil)
and (defvar ido-choice-list nil).  These changes aren't necessarily bad,
but I suspect the change is either unneeded or hiding a bug.


        Stefan

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I confirm the original bug report. I propose here a different fix for
>>> both ido and iswitch. It is much simpler and more efficient.
>> On the minus side, your patch:
>>   - is less generic; Óscar's will work for other packages that do
>> dynamic renaming of buffers (yes, there are none on the Emacs
>> distribution, but there's a lot of elisp code out there).
>>   - makes ido depend on a feature of uniquify, which it didn't before.
>
> Even more problematic: it changes the behavior of uniquify.
> Can someone install Oscar's patch?

It was installed long time ago.

> BTW, I don't understand why his patch changes (defvar ido-cur-list)
> and (defvar ido-choice-list) into (defvar ido-cur-list nil)
> and (defvar ido-choice-list nil).  These changes aren't necessarily bad,
> but I suspect the change is either unneeded or hiding a bug.

I was unable to obtain a solid insight on how ido managed the list of
buffers. It looked quite complicated and dispersed to me. So tried some
code, tested (with Juanma's help) and submitted the patch. The last
patch posted by Leo looks great because it is so simple, but IIRC I
considered some simple ways of fixing the bug and they introduced
strange bugs and annoyances(*), so maybe my fix doesn't contain so much
gratuitous code at it seems, or maybe Leo is right and I was unlucky
with my attempts at fixing the bug with a simple change.

* One of those annoyances was changing the "next" item on the list of
  buffers once you kill the first one, something that I find
  confusing. Right now ido may change the order of the buffers after a
  kill, but the previous second item appears as the first item on the
  new list.

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

On 5 May 2010 20:09, Óscar Fuentes <ofv@wanadoo.es> wrote:
> It was installed long time ago.
>
>> BTW, I don't understand why his patch changes (defvar ido-cur-list)
>> and (defvar ido-choice-list) into (defvar ido-cur-list nil)
>> and (defvar ido-choice-list nil).  These changes aren't necessarily bad,
>> but I suspect the change is either unneeded or hiding a bug.
>
> I was unable to obtain a solid insight on how ido managed the list of
> buffers. It looked quite complicated and dispersed to me. So tried some
> code, tested (with Juanma's help) and submitted the patch. The last
> patch posted by Leo looks great because it is so simple, but IIRC I
> considered some simple ways of fixing the bug and they introduced
> strange bugs and annoyances(*), so maybe my fix doesn't contain so much
> gratuitous code at it seems, or maybe Leo is right and I was unlucky
> with my attempts at fixing the bug with a simple change.
>
> * One of those annoyances was changing the "next" item on the list of
>  buffers once you kill the first one, something that I find
>  confusing. Right now ido may change the order of the buffers after a
>  kill, but the previous second item appears as the first item on the
>  new list.
>

Could you try this function to see if it does what you want? Also let
me know what is missing. I will try fix it today. Thanks.

(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."
  (interactive)
  (if (not (eobp))
      (delete-region (point) (line-end-position))
    (let ((enable-recursive-minibuffers t)
	  (buf (ido-name (car ido-matches)))
          (nextbuf (get-buffer (cadr ido-matches))))
      (when buf
	(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.
          (setq ido-default-item (buffer-name nextbuf))
	  (setq ido-text-init ido-text)
	  (setq ido-exit 'refresh)
          (exit-minibuffer))))))

Leo

Re: PATCH: Fix IDO interaction with uniquify.el

[Leo]

> (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."
>  (interactive)
>  (if (not (eobp))
>      (delete-region (point) (line-end-position))
>    (let ((enable-recursive-minibuffers t)
>          (buf (ido-name (car ido-matches)))
>          (nextbuf (get-buffer (cadr ido-matches))))
>      (when buf
>        (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.
>          (setq ido-default-item (buffer-name nextbuf))
>          (setq ido-text-init ido-text)
>          (setq ido-exit 'refresh)
>          (exit-minibuffer))))))

Robustified version. I think we are close to a fix.

(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."
  (interactive)
  (if (not (eobp))
      (delete-region (point) (line-end-position))
    (let ((enable-recursive-minibuffers t)
	  (buf (ido-name (car ido-matches)))
          (nextbuf (cadr ido-matches)))
      (if (and nextbuf (buffer-live-p nextbuf))
          (setq nextbuf (get-buffer nextbuf))
        (setq nextbuf nil))
      (when buf
	(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.
          (and nextbuf (setq ido-default-item (buffer-name nextbuf)))
	  (setq ido-text-init ido-text)
	  (setq ido-exit 'refresh)
          (exit-minibuffer))))))

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

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

> Robustified version. I think we are close to a fix.

[snip]

This days I'm too busy and have no time for testing your proposed
change, but I see that the complexity of your fix is growing to the
point that only efficiency possibly remains as its justification. Are we
sure we really need that efficiency?

Re: PATCH: Fix IDO interaction with uniquify.el

[Juri Linkov]

> (defun ido-kill-buffer-at-head ()
>   "Kill the buffer at the head of `ido-matches'.

A better function name would be `ido-shoot-buffer-at-head' :-)

Seriously I wonder how virtual buffers could be used for restoring
window configurations.  When a buffer is killed, after restoring
a window configuration we need to display some virtual buffer in place
of the killed buffer.

I don't understand if ido virtual buffers are intended for something
like that?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: PATCH: Fix IDO interaction with uniquify.el

[Stefan Monnier]

>> * One of those annoyances was changing the "next" item on the list of
>>  buffers once you kill the first one, something that I find
>>  confusing. Right now ido may change the order of the buffers after a
>>  kill, but the previous second item appears as the first item on the
>>  new list.

I don't understand.  So you're saying there's a problem (killing may
change the order) but the current code has a fix for it (at least for
the "first" element of the remaining list).  Right?

> Could you try this function to see if it does what you want? Also let
> me know what is missing. I will try fix it today. Thanks.

What is it trying to fix?  And how?

>     (let ((enable-recursive-minibuffers t)

Why?  There doesn't seem to be any call to a recursive minibuffer
in sight.


        Stefan

Re: PATCH: Fix IDO interaction with uniquify.el

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> * One of those annoyances was changing the "next" item on the list of
>>>  buffers once you kill the first one, something that I find
>>>  confusing. Right now ido may change the order of the buffers after a
>>>  kill, but the previous second item appears as the first item on the
>>>  new list.
>
> I don't understand.  So you're saying there's a problem (killing may
> change the order) but the current code has a fix for it (at least for
> the "first" element of the remaining list).  Right?

Yes.

Re: PATCH: Fix IDO interaction with uniquify.el

[Kim F. Storm]

Óscar Fuentes <ofv@wanadoo.es> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> * One of those annoyances was changing the "next" item on the list of
>>>>  buffers once you kill the first one, something that I find
>>>>  confusing. Right now ido may change the order of the buffers after a
>>>>  kill, but the previous second item appears as the first item on the
>>>>  new list.
>>
>> I don't understand.  So you're saying there's a problem (killing may
>> change the order) but the current code has a fix for it (at least for
>> the "first" element of the remaining list).  Right?
>
> Yes.

There is a long-standing and annoying bug in ido: 

Depending on the ido settings, buffers available, and matching items,
simply doing next or prev may change the order of the remaining items.

I'm working on a fix, but this is unrelated to the current discussion.

At least these versions seem to work better than before.

(defun ido-next-match ()
  "Put first element of `ido-matches' at the end of the list."
  (interactive)
  (if ido-matches
      (let ((next (cadr ido-matches)))
	(setq ido-cur-list (ido-chop ido-cur-list next))
	(setq ido-matches (ido-chop ido-matches next))
	(setq ido-rescan nil))))

(defun ido-prev-match ()
  "Put last element of `ido-matches' at the front of the list."
  (interactive)
  (if ido-matches
      (let ((prev (car (last ido-matches))))
	(setq ido-cur-list (ido-chop ido-cur-list prev))
	(setq ido-matches (ido-chop ido-matches prev))
	(setq ido-rescan nil))))


-- 
Kim F. Storm  http://www.cua.dk