Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stefan Monnier]

> -  (let ((map minibuffer-local-completion-map))
> -    (define-key map " " nil)
> +  (let ((minibuffer-local-completion-map
> +         (copy-keymap minibuffer-local-completion-map)))
> +    (define-key minibuffer-local-completion-map " " 'self-insert-command)

Why bind it to `self-insert-command` rather than to nil?


        Stefan


PS: You can avoid copying with something like

  (let ((minibuffer-local-completion-map
         (let ((map (make-sparse-keymap)))
           (set-parent-keymap map minibuffer-local-completion-map)
           (define-key minibuffer-local-completion-map
                       " " 'self-insert-command))))

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stephen Berman]

On Tue, 14 Jun 2016 22:12:01 -0400 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> -  (let ((map minibuffer-local-completion-map))
>> -    (define-key map " " nil)
>> +  (let ((minibuffer-local-completion-map
>> +         (copy-keymap minibuffer-local-completion-map)))
>> +    (define-key minibuffer-local-completion-map " " 'self-insert-command)
>
> Why bind it to `self-insert-command` rather than to nil?

When I found the todo-mode bug I grepped the lisp/ tree for
minibuffer-local-completion-map and saw that in all packages that want
SPC to insert " " in the minibuffer it's bound to self-insert-command,
so I concluded that was best practice and changed it accordingly.  The
reasoning seems to be that if someone rebinds SPC in the global map, it
still should be used to insert " " in these cases.  But I suppose that
whoever does rebind SPC in the global map still must have some binding
for inserting " ", so maybe packages should respect that rebinding and
just use nil; is that what you suggest?  (Oh, now I see that in
minibuffer-local-filename-completion-map you did bind it to nil, so I
guess that answers the question.)  Is that a general recommendation,
e.g. to make the `?' key insert `?' in the minibuffer also bind it to
nil?

> PS: You can avoid copying with something like
>
>   (let ((minibuffer-local-completion-map
>          (let ((map (make-sparse-keymap)))
>            (set-parent-keymap map minibuffer-local-completion-map)
>            (define-key minibuffer-local-completion-map
>                        " " 'self-insert-command))))

It seems that currently slightly more packages use copy-keymap
(calc-store, mh-alias, mh-seq, org, supercite, viper-ex) than
set-keymap-parent (cc-styles, mm-decode, crm, minibuffer) to override
minibuffer-local-completion-map bindings (though for all uses in in
lisp/ I count 83 occurrences of "(copy-keymap" and 165 of
"(set-keymap-parent").  Are you saying it is generally better to use the
latter?  If so, when is it better to use the former?

Steve Berman

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stefan Monnier]

>>> -  (let ((map minibuffer-local-completion-map))
>>> -    (define-key map " " nil)
>>> +  (let ((minibuffer-local-completion-map
>>> +         (copy-keymap minibuffer-local-completion-map)))
>>> +    (define-key minibuffer-local-completion-map " " 'self-insert-command)
>> Why bind it to `self-insert-command` rather than to nil?
> Is that a general recommendation,
> e.g. to make the `?' key insert `?' in the minibuffer also bind it to
> nil?

The question is: do you want to hide the "SPC is completion" binding, or
do you want to force SPC to insert a space.

> I guess that answers the question.)

Right, IMO what we want here is to hide the "SPC is completion" binding.
Also, the old code used nil (as can be seen in the chunk of patch still
quoted above).

> Are you saying it is generally better to use the latter?

Yes, it's more efficient.

> If so, when is it better to use the former?

Never.  Copying is a bad idea.


        Stefan

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stephen Berman]

On Wed, 15 Jun 2016 09:16:22 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>>>> -  (let ((map minibuffer-local-completion-map))
>>>> -    (define-key map " " nil)
>>>> +  (let ((minibuffer-local-completion-map
>>>> +         (copy-keymap minibuffer-local-completion-map)))
>>>> +    (define-key minibuffer-local-completion-map " " 'self-insert-command)
>>> Why bind it to `self-insert-command` rather than to nil?
>> Is that a general recommendation,
>> e.g. to make the `?' key insert `?' in the minibuffer also bind it to
>> nil?
>
> The question is: do you want to hide the "SPC is completion" binding, or
> do you want to force SPC to insert a space.
>
>> I guess that answers the question.)
>
> Right, IMO what we want here is to hide the "SPC is completion" binding.
> Also, the old code used nil (as can be seen in the chunk of patch still
> quoted above).

Ok.  I suppose that's also the case for most or all of the other
packages that bind SPC to self-insert-command in the minibuffer, so they
should probably also be changed in due course.

>> Are you saying it is generally better to use the latter?
>
> Yes, it's more efficient.
>
>> If so, when is it better to use the former?
>
> Never.  Copying is a bad idea.

Then shouldn't copy-keymap be obsoleted (or at least deprecated, also in
the documentation) and existing uses in Emacs changed?

I'll revert the key binding to nil.  I assume it's also all right to
replace the use of copy-keymap by set-keymap-parent in emacs-25, since
copy-keymap was just introduced as a fix there by this commit, but I'll
wait for explicit approval (Eli, John W?).

Thanks.

Steve Berman

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Wed, 15 Jun 2016 17:09:53 +0200
> Cc: emacs-devel@gnu.org
> 
> I assume it's also all right to replace the use of copy-keymap by
> set-keymap-parent in emacs-25, since copy-keymap was just introduced
> as a fix there by this commit, but I'll wait for explicit approval
> (Eli, John W?).

Fine with me.

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[John Wiegley]

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

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

>> I assume it's also all right to replace the use of copy-keymap by
>> set-keymap-parent in emacs-25, since copy-keymap was just introduced
>> as a fix there by this commit, but I'll wait for explicit approval
>> (Eli, John W?).

> Fine with me.

Also, at this point in the game, if it's OK with Eli, it's OK with me. I
consider him the "release manager" for 25.1.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stephen Berman]

On Wed, 15 Jun 2016 10:31:47 -0700 John Wiegley <jwiegley@gmail.com> wrote:

>>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> I assume it's also all right to replace the use of copy-keymap by
>>> set-keymap-parent in emacs-25, since copy-keymap was just introduced
>>> as a fix there by this commit, but I'll wait for explicit approval
>>> (Eli, John W?).
>
>> Fine with me.
>
> Also, at this point in the game, if it's OK with Eli, it's OK with me. I
> consider him the "release manager" for 25.1.

Thanks, done as commit 5d4d8a3.

Steve Berman

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Noam Postavsky]

On Wed, Jun 15, 2016 at 2:10 PM, Stephen Berman <stephen.berman@gmx.net> wrote:
>
> Thanks, done as commit 5d4d8a3.

I think it does nothing but bind minibuffer-local-completion-map to
nil, since define-key doesn't return the map. You should have

  (let ((minibuffer-local-completion-map
         (let ((map (make-sparse-keymap)))
           (set-keymap-parent map minibuffer-local-completion-map)
           (define-key map " " nil)
           map))) ; <---------- use the map
    ...

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stephen Berman]

On Wed, 15 Jun 2016 23:04:27 -0400 Noam Postavsky <npostavs@users.sourceforge.net> wrote:

> On Wed, Jun 15, 2016 at 2:10 PM, Stephen Berman <stephen.berman@gmx.net> wrote:
>>
>> Thanks, done as commit 5d4d8a3.
>
> I think it does nothing but bind minibuffer-local-completion-map to
> nil, since define-key doesn't return the map. You should have
>
>   (let ((minibuffer-local-completion-map
>          (let ((map (make-sparse-keymap)))
>            (set-keymap-parent map minibuffer-local-completion-map)
>            (define-key map " " nil)
>            map))) ; <---------- use the map

Thanks for catching that.  Fix pushed to emacs-25 as commit 2317c61.  (I
had tested my previous change before committing it, but only that SPC
worked as intended (because it is bound to the global map), and
neglected the other minibuffer bindings.)

Steve Berman

Generalizing make-composed-keymap (was: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).)

[Stefan Monnier]

BTW, currently make-composed-keymap takes a list of maps as argument,
but I'm beginning to think it would make sense to also accept (KEY
. CMD) elements in that list, so that we can just say

    (make-composed-keymap
     '((" " my-space-replacement)
       ("?" nil))                ; Just hide the old ? binding.
     my-parent-map)

instead of

    (let ((map (make-sparse-keymap)))
      (set-keymap-parent map my-parent-map)
      (define-key map " " 'my-space-replacement)
      (define-key map "?" nil)
      map)

WDYT?


        Stefan

Re: Generalizing make-composed-keymap

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]

On 2016-06-15 11:23, Stefan Monnier wrote:
> BTW, currently make-composed-keymap takes a list of maps as argument,
> but I'm beginning to think it would make sense to also accept (KEY
> . CMD) elements in that list, so that we can just say
> 
>     (make-composed-keymap
>      '((" " my-space-replacement)
>        ("?" nil))                ; Just hide the old ? binding.
>      my-parent-map)
> 
> instead of
> 
>     (let ((map (make-sparse-keymap)))
>       (set-keymap-parent map my-parent-map)
>       (define-key map " " 'my-space-replacement)
>       (define-key map "?" nil)
>       map)
> 
> WDYT?

What about adding a `parent' option to make-sparse-keymap instead? That could help with discoverability.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Generalizing make-composed-keymap

[Stefan Monnier]

>> BTW, currently make-composed-keymap takes a list of maps as argument,
>> but I'm beginning to think it would make sense to also accept (KEY
>> . CMD) elements in that list, so that we can just say
[...]
> What about adding a `parent' option to make-sparse-keymap instead?
> That could help with discoverability.

Hmm... indeed I guess we could even merge make-sparse-keymap and
make-composed-keymap: if the first arg is a string, treat it as the
"menu name", otherwise treat it as a list of maps or list of bindings.


        Stefan

Re: [Emacs-diffs] emacs-25 d7084f2: Fix todo-mode use of minibuffer completion keymap (bug#23695).

[Stefan Monnier]

> Then shouldn't copy-keymap be obsoleted (or at least deprecated, also in
> the documentation) and existing uses in Emacs changed?

I don't know about obsoleting it (set-kemap-parent does something
different, so it doesn't make it obsolete).  But yes, its doc should
probably point out that it's usually the wrong tool.  I just did that
in trunk.


        Stefan