GNU bug report logs - #17000
setup ido keymaps only once

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17000 in the body.
You can then email your comments to 17000 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jonas Bernoulli <jonas <at> bernoul.li>
To: bug-gnu-emacs <at> gnu.org
Subject: setup ido keymaps only once
Date: Wed, 12 Mar 2014 20:35:16 +0100

Currently the ido keymaps are hard to modify because they are recreated
every time `ido-completing-read' is used.  This forces users to add a
function to `ido-minibuffer-setup' and do their `define-key's there.

I believe it is safe for the "base" ido keymaps to set the value when
the variable is defined, instead of delaying this until ido is actually
used, and then redoing that work whenever `ido-completing-read' is
called.

These are the keymaps which are currently set in `ido-init-completion-maps':
  ido-common-completion-map
  ido-file-completion-map
  ido-file-dir-completion-map
  ido-buffer-completion-map

`ido-completion-map' is different, it's a kind of "current-local-map".
Its value is set in `ido-setup-completion-map' by creating a new sparse
keymap, setting its parent to one of the above keymaps (depending on
what is currently being completed) and then possibly making some
adjustments depending on `ido-context-switch-command' and `viper-mode'.

`ido-setup-completion-map' is called every time `ido-read-internal' is
called, i.e. every time the user actually uses ido completion.  Therefor
`ido-completion-map' is set every time ido completion is used and so it
is safe to modify it using `ido-minibuffer-setup-hook' for just this one
call.

`ido-init-completion-maps' is *not* called every time ido completion is
used.  It is called only in `ido-common-initialization' which is called
only in `ido-mode' and `ido-completion-read', but not in
`ido-read-internal'.  The reason why `ido-completion-read' has to do the
common initialization is simple: it may be used by users and packages to
complete certain things without also using ido completion for everything
`ido-mode' itself supports.

Besides the initialization of the keymaps the "common initialization"
also includes adding functions to hooks.  I am arguing that the latter
should stay but that the keymaps shouldn't be recreated here and that it
is backward compatible to make that change.

While it is possible that some third-party code modifies one of the
`ido-*-completion-map's by using `ido-minibuffer-setup', that would be
a bug.  Instead the "final keymap", `ido-completion-map', should be
modified.  Even without the proposed change modifying one of the
`ido-*-completion-map's when using `ido-completing-read' with the
intention of having that only affect the current invocation would
actually affect later uses of ido completion because only
`ido-completing-read' re-runs `ido-init-completion-maps',
`ido-read-internal' does *not* do that.  Therefore changes to these
keymaps would remain in effect until the next time `ido-completing-read'
is used, uses of `ido-read-internal' that happen in between would use
the modified map.

  Best regards,
  Jonas

---

diff --git a/lisp/ido.el b/lisp/ido.el
index b16ab1f..b4a3b28 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -978,25 +978,96 @@ The fallback command is passed as an argument to the functions."
   :type 'hook
   :group 'ido)
 
-;;; Internal Variables
-
-;; Persistent variables
-
-(defvar ido-completion-map nil
-  "Currently active keymap for Ido commands.")
+;; Keymaps
 
-(defvar ido-common-completion-map nil
+(defvar ido-common-completion-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-a" 'ido-toggle-ignore)
+    (define-key map "\C-c" 'ido-toggle-case)
+    (define-key map "\C-e" 'ido-edit-input)
+    (define-key map "\t" 'ido-complete)
+    (define-key map " " 'ido-complete-space)
+    (define-key map "\C-j" 'ido-select-text)
+    (define-key map "\C-m" 'ido-exit-minibuffer)
+    (define-key map "\C-p" 'ido-toggle-prefix)
+    (define-key map "\C-r" 'ido-prev-match)
+    (define-key map "\C-s" 'ido-next-match)
+    (define-key map [?\C-.] 'ido-next-match)
+    (define-key map [?\C-,] 'ido-prev-match)
+    (define-key map "\C-t" 'ido-toggle-regexp)
+    (define-key map "\C-z" 'ido-undo-merge-work-directory)
+    (define-key map [(control ?\s)] 'ido-restrict-to-matches)
+    (define-key map [(meta ?\s)] 'ido-take-first-match)
+    (define-key map [(control ?@)] 'ido-restrict-to-matches)
+    (define-key map [right] 'ido-next-match)
+    (define-key map [left] 'ido-prev-match)
+    (define-key map "?" 'ido-completion-help)
+    ;; Magic commands.
+    (define-key map "\C-b" 'ido-magic-backward-char)
+    (define-key map "\C-f" 'ido-magic-forward-char)
+    (define-key map "\C-d" 'ido-magic-delete-char)
+    (set-keymap-parent map minibuffer-local-map)
+    (setq ido-common-completion-map map))
   "Keymap for all Ido commands.")
 
-(defvar ido-file-completion-map nil
+(defvar ido-file-completion-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-k" 'ido-delete-file-at-head)
+    (define-key map "\C-o" 'ido-copy-current-word)
+    (define-key map "\C-w" 'ido-copy-current-file-name)
+    (define-key map [(meta ?l)] 'ido-toggle-literal)
+    (set-keymap-parent map ido-file-dir-completion-map)
+    (setq ido-file-completion-map map))
   "Keymap for Ido file commands.")
 
-(defvar ido-file-dir-completion-map nil
+(defvar ido-file-dir-completion-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-x\C-b" 'ido-enter-switch-buffer)
+    (define-key map "\C-x\C-f" 'ido-fallback-command)
+    (define-key map "\C-x\C-d" 'ido-enter-dired)
+    (define-key map [down] 'ido-next-match-dir)
+    (define-key map [up]   'ido-prev-match-dir)
+    (define-key map [(meta up)] 'ido-prev-work-directory)
+    (define-key map [(meta down)] 'ido-next-work-directory)
+    (define-key map [backspace] 'ido-delete-backward-updir)
+    (define-key map "\d"        'ido-delete-backward-updir)
+    (define-key map [remap delete-backward-char] 'ido-delete-backward-updir) ; BS
+    (define-key map [remap backward-kill-word] 'ido-delete-backward-word-updir)  ; M-DEL
+
+    (define-key map [(control backspace)] 'ido-up-directory)
+    (define-key map "\C-l" 'ido-reread-directory)
+    (define-key map [(meta ?d)] 'ido-wide-find-dir-or-delete-dir)
+    (define-key map [(meta ?b)] 'ido-push-dir)
+    (define-key map [(meta ?v)] 'ido-push-dir-first)
+    (define-key map [(meta ?f)] 'ido-wide-find-file-or-pop-dir)
+    (define-key map [(meta ?k)] 'ido-forget-work-directory)
+    (define-key map [(meta ?m)] 'ido-make-directory)
+    (define-key map [(meta ?n)] 'ido-next-work-directory)
+    (define-key map [(meta ?o)] 'ido-prev-work-file)
+    (define-key map [(meta control ?o)] 'ido-next-work-file)
+    (define-key map [(meta ?p)] 'ido-prev-work-directory)
+    (define-key map [(meta ?s)] 'ido-merge-work-directories)
+    (set-keymap-parent map ido-common-completion-map)
+    (setq ido-file-dir-completion-map map))
   "Keymap for Ido file and directory commands.")
 
-(defvar ido-buffer-completion-map nil
+(defvar ido-buffer-completion-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-x\C-f" 'ido-enter-find-file)
+    (define-key map "\C-x\C-b" 'ido-fallback-command)
+    (define-key map "\C-k" 'ido-kill-buffer-at-head)
+    (define-key map "\C-o" 'ido-toggle-virtual-buffers)
+    (set-keymap-parent map ido-common-completion-map)
+    (setq ido-buffer-completion-map map))
   "Keymap for Ido buffer commands.")
 
+;;; Internal Variables
+
+;; Persistent variables
+
+(defvar ido-completion-map nil
+  "Currently active keymap for Ido commands.")
+
 (defvar  ido-file-history nil
   "History of files selected using `ido-find-file'.")
 
@@ -1503,7 +1574,6 @@ Removes badly formatted data and ignored directories."
   (ido-save-history))
 
 (defun ido-common-initialization ()
-  (ido-init-completion-maps)
   (add-hook 'minibuffer-setup-hook 'ido-minibuffer-setup)
   (add-hook 'choose-completion-string-functions 'ido-choose-completion-string))
 
@@ -1597,90 +1667,22 @@ This function also adds a hook to the minibuffer."
 
 
 ;;; IDO KEYMAP
-(defun ido-init-completion-maps ()
-  "Set up the completion keymaps used by Ido."
-
-  ;; Common map
-  (let ((map (make-sparse-keymap)))
-    (define-key map "\C-a" 'ido-toggle-ignore)
-    (define-key map "\C-c" 'ido-toggle-case)
-    (define-key map "\C-e" 'ido-edit-input)
-    (define-key map "\t" 'ido-complete)
-    (define-key map " " 'ido-complete-space)
-    (define-key map "\C-j" 'ido-select-text)
-    (define-key map "\C-m" 'ido-exit-minibuffer)
-    (define-key map "\C-p" 'ido-toggle-prefix)
-    (define-key map "\C-r" 'ido-prev-match)
-    (define-key map "\C-s" 'ido-next-match)
-    (define-key map [?\C-.] 'ido-next-match)
-    (define-key map [?\C-,] 'ido-prev-match)
-    (define-key map "\C-t" 'ido-toggle-regexp)
-    (define-key map "\C-z" 'ido-undo-merge-work-directory)
-    (define-key map [(control ?\s)] 'ido-restrict-to-matches)
-    (define-key map [(meta ?\s)] 'ido-take-first-match)
-    (define-key map [(control ?@)] 'ido-restrict-to-matches)
-    (define-key map [right] 'ido-next-match)
-    (define-key map [left] 'ido-prev-match)
-    (define-key map "?" 'ido-completion-help)
-    ;; Magic commands.
-    (define-key map "\C-b" 'ido-magic-backward-char)
-    (define-key map "\C-f" 'ido-magic-forward-char)
-    (define-key map "\C-d" 'ido-magic-delete-char)
-    (set-keymap-parent map minibuffer-local-map)
-    (setq ido-common-completion-map map))
-
-  ;; File and directory map
-  (let ((map (make-sparse-keymap)))
-    (define-key map "\C-x\C-b" 'ido-enter-switch-buffer)
-    (define-key map "\C-x\C-f" 'ido-fallback-command)
-    (define-key map "\C-x\C-d" 'ido-enter-dired)
-    (define-key map [down] 'ido-next-match-dir)
-    (define-key map [up]   'ido-prev-match-dir)
-    (define-key map [(meta up)] 'ido-prev-work-directory)
-    (define-key map [(meta down)] 'ido-next-work-directory)
-    (define-key map [backspace] 'ido-delete-backward-updir)
-    (define-key map "\d"        'ido-delete-backward-updir)
-    (define-key map [remap delete-backward-char] 'ido-delete-backward-updir) ; BS
-    (define-key map [remap backward-kill-word] 'ido-delete-backward-word-updir)  ; M-DEL
-
-    (define-key map [(control backspace)] 'ido-up-directory)
-    (define-key map "\C-l" 'ido-reread-directory)
-    (define-key map [(meta ?d)] 'ido-wide-find-dir-or-delete-dir)
-    (define-key map [(meta ?b)] 'ido-push-dir)
-    (define-key map [(meta ?v)] 'ido-push-dir-first)
-    (define-key map [(meta ?f)] 'ido-wide-find-file-or-pop-dir)
-    (define-key map [(meta ?k)] 'ido-forget-work-directory)
-    (define-key map [(meta ?m)] 'ido-make-directory)
-    (define-key map [(meta ?n)] 'ido-next-work-directory)
-    (define-key map [(meta ?o)] 'ido-prev-work-file)
-    (define-key map [(meta control ?o)] 'ido-next-work-file)
-    (define-key map [(meta ?p)] 'ido-prev-work-directory)
-    (define-key map [(meta ?s)] 'ido-merge-work-directories)
-    (set-keymap-parent map ido-common-completion-map)
-    (setq ido-file-dir-completion-map map))
 
-  ;; File only map
-  (let ((map (make-sparse-keymap)))
-    (define-key map "\C-k" 'ido-delete-file-at-head)
-    (define-key map "\C-o" 'ido-copy-current-word)
-    (define-key map "\C-w" 'ido-copy-current-file-name)
-    (define-key map [(meta ?l)] 'ido-toggle-literal)
-    (set-keymap-parent map ido-file-dir-completion-map)
-    (setq ido-file-completion-map map))
+(defun ido-setup-completion-map ()
+  "Set up the keymap for Ido.
 
-  ;; Buffer map
-  (let ((map (make-sparse-keymap)))
-    (define-key map "\C-x\C-f" 'ido-enter-find-file)
-    (define-key map "\C-x\C-b" 'ido-fallback-command)
-    (define-key map "\C-k" 'ido-kill-buffer-at-head)
-    (define-key map "\C-o" 'ido-toggle-virtual-buffers)
-    (set-keymap-parent map ido-common-completion-map)
-    (setq ido-buffer-completion-map map)))
+Create a keymap and depending on what is being completed set its
+parent to one of:
 
+  `ido-common-completion-map'
+  `ido-file-dir-completion-map'
+  `ido-file-completion-map'
+  `ido-buffer-completion-map'
 
-(defun ido-setup-completion-map ()
-  "Set up the keymap for Ido."
+If option `ido-context-switch-command' is non-nil or `viper-mode'
+is enabled some keybindings are changed in the keymap.
 
+Finally `ido-completion-map' is bound to the keymap."
   ;; generated every time so that it can inherit new functions.
   (let ((map (make-sparse-keymap))
 	(viper-p (if (boundp 'viper-mode) viper-mode)))

Message #8 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jonas Bernoulli <jonas <at> bernoul.li>
Cc: 17000 <at> debbugs.gnu.org
Subject: Re: bug#17000: setup ido keymaps only once
Date: Wed, 12 Mar 2014 19:36:06 -0400

> Currently the ido keymaps are hard to modify because they are recreated
> every time `ido-completing-read' is used.

Indeed, this is wrong.

> I believe it is safe for the "base" ido keymaps to set the value when
> the variable is defined, instead of delaying this until ido is actually
> used, and then redoing that work whenever `ido-completing-read' is
> called.

Agreed.

> -(defvar ido-common-completion-map nil
> +(defvar ido-common-completion-map
> +  (let ((map (make-sparse-keymap)))
[...]
> +    (set-keymap-parent map minibuffer-local-map)
> +    (setq ido-common-completion-map map))

This last line should just be "map)".  Same for the other maps.

Looks good.  Please apply after the trunk re-opens.


        Stefan "Beware: I don't use IDO and don't know its code"

Message #11 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Jonas Bernoulli <jonas <at> bernoul.li>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17000 <at> debbugs.gnu.org
Subject: Re: bug#17000: setup ido keymaps only once
Date: Thu, 13 Mar 2014 16:35:15 +0100

> This last line should just be "map)".  Same for the other maps.
> Looks good.  Please apply after the trunk re-opens.

Once the trunk re-opens I will submit a new patch with this fixed
and which includes the necessary changelog entry.

>         Stefan "Beware: I don't use IDO and don't know its code"

  Jonas "Who was bothered by this so much he didn't use IDO until now" :-)

Message #14 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
To: Jonas Bernoulli <jonas <at> bernoul.li>
Cc: 17000 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#17000: setup ido keymaps only once
Date: Tue, 09 Dec 2014 00:39:15 +0100

Jonas Bernoulli <jonas <at> bernoul.li> writes:

>> This last line should just be "map)".  Same for the other maps.
>> Looks good.  Please apply after the trunk re-opens.
>
> Once the trunk re-opens I will submit a new patch with this fixed
> and which includes the necessary changelog entry.
>
>>         Stefan "Beware: I don't use IDO and don't know its code"
>
>   Jonas "Who was bothered by this so much he didn't use IDO until now" :-)

The trunk is open now if you have an updated version of this patch...

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

Message #17 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Jonas Bernoulli <jonas <at> bernoul.li>
To: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
Cc: 17000 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#17000: setup ido keymaps only once
Date: Wed, 10 Dec 2014 10:15:08 +0100

Lars Magne Ingebrigtsen <larsi <at> gnus.org> writes:

> Jonas Bernoulli <jonas <at> bernoul.li> writes:
>
>>> This last line should just be "map)".  Same for the other maps.
>>> Looks good.  Please apply after the trunk re-opens.
>>
>> Once the trunk re-opens I will submit a new patch with this fixed
>> and which includes the necessary changelog entry.
>>
>>>         Stefan "Beware: I don't use IDO and don't know its code"
>>
>>   Jonas "Who was bothered by this so much he didn't use IDO until now" :-)
>
> The trunk is open now if you have an updated version of this patch...

Yes I know, thanks.  This is fairly high up on my todo list.
I'll test a bit and then post a new version.

Message #20 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Jonas Bernoulli <jonas <at> bernoul.li>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17000 <at> debbugs.gnu.org
Subject: Re: bug#17000: setup ido keymaps only once
Date: Mon, 22 Dec 2014 19:53:40 +0100

[Message part 1 (text/plain, inline)]

> Once the trunk re-opens I will submit a new patch with this fixed
> and which includes the necessary changelog entry.

Here we go.  I hope I got the metadata mostly right.

I have used this, along with various third-party ido extensions,
for a while now (with 24.4 though) and without any problems.

  Best regards,
  Jonas

[0001-Define-ido-keymaps-when-the-variables-are-defined.patch (text/x-diff, attachment)]

Message #23 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17000 <at> debbugs.gnu.org, Jonas Bernoulli <jonas <at> bernoul.li>
Subject: Re: bug#17000: setup ido keymaps only once
Date: Tue, 23 Dec 2014 10:07:53 +0800

On 2014-03-13 07:36 +0800, Stefan Monnier wrote:
> Indeed, this is wrong.

Ido's author is not an idiot and AFAIK he is responsive to questions on
ido. So maybe ask him first for why it was designed that way ;)

Leo

Message #26 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Leo Liu <sdl.web <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17000 <at> debbugs.gnu.org, Jonas Bernoulli <jonas <at> bernoul.li>,
 "Kim F. Storm" <storm <at> cua.dk>
Subject: Re: bug#17000: setup ido keymaps only once
Date: Wed, 24 Dec 2014 19:11:43 +0200

On 12/23/2014 04:07 AM, Leo Liu wrote:

> Ido's author is not an idiot and AFAIK he is responsive to questions on
> ido. So maybe ask him first for why it was designed that way ;)

Kim, any particular reason it was written this way?

Message #29 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Kim Storm <storm <at> cua.dk>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: "17000 <at> debbugs.gnu.org" <17000 <at> debbugs.gnu.org>,
 Jonas Bernoulli <jonas <at> bernoul.li>, Leo Liu <sdl.web <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#17000: setup ido keymaps only once
Date: Thu, 25 Dec 2014 23:44:58 +0100

[Message part 1 (text/plain, inline)]

As far as I remember, that part of ido has grown out of code inherited from iswitchb, so I probably just didn't see a need to change something that worked ok for ido.

Iswitchb had this comment in the keymap setup function:
;; generated every time so that it can inherit new functions.
The new patches seems fine to me.

Regards Kim 

> Den 24/12/2014 kl. 18.11 skrev Dmitry Gutov <dgutov <at> yandex.ru>:
> 
>> On 12/23/2014 04:07 AM, Leo Liu wrote:
>> 
>> Ido's author is not an idiot and AFAIK he is responsive to questions on
>> ido. So maybe ask him first for why it was designed that way ;)
> 
> Kim, any particular reason it was written this way?

[Message part 2 (text/html, inline)]

Message #32 received at 17000 <at> debbugs.gnu.org (full text, mbox):

From: Oleh Krehel <o.krehel <at> tue.nl>
To: 17000 <at> debbugs.gnu.org
Subject: Re: bug#17000: setup ido keymaps only once
Date: Sun, 18 Jan 2015 16:17:27 +0100

Hello,

I have been using Jonas' patch for a week now.
It's working fine.

regards,
Oleh

Message #37 received at 17000-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Oleh Krehel <o.krehel <at> tue.nl>, 17000-done <at> debbugs.gnu.org,
 Jonas Bernoulli <jonas <at> bernoul.li>
Subject: Re: bug#17000: setup ido keymaps only once
Date: Mon, 19 Jan 2015 13:58:15 +0200

Version: 25.1

On 01/18/2015 05:17 PM, Oleh Krehel wrote:

> I have been using Jonas' patch for a week now.
> It's working fine.

Thanks for checking, installed.

Thanks, Jonas!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.