Re: bizarre problem with minor mode defined using define-minor-mode

[ken manheimer]

On Sun, Jan 16, 2011 at 1:46 AM, Stefan Monnier
<address@hidden> wrote:
> [...]
>>>> in fact, removing the fset leads to the same problems with the
>>>> regular, defun'ed allout-mode that i have been seeing with the
>>>> byte-compiled define-minor-mode defined allout mode.
>>> That's what I would expect, since I think this fset is a no-op.
>> it's not a no-op!  as i said before, removing the fset changed
>> behavior.  as i describe above, there's an explanation.
>
> Then, I'm lost.

andreas schwab pointed out the place.

my mistake was assuming that define-minor-mode would notice that the
intended minor-mode-map-alist entry already existed and leave it be.

>[...]
> So if you use `allout-mode-map', it will use the value of that variable,
> and if you use something else, it will turn it into a keymap, set it as
> default value of allout-mode-map, and then use the value of
> allout-mode-map.  Since the allout-mode-map is already defined before,
> this ends up having no effect.  Here's a relevant sample of IELM
> session:

(IELM looks good - thanks for mentioning it.  likewise, macroexpand is
very helpful - i should have figured such a thing existed, and sought
it to check what define-minor-mode does.)

> I suggest to stay away from the :keymap argument.  Instead, do it this way:
>
> (defvar allout-mode--map <blabla>)
> (defalias 'allout-mode-map allout-mode--map)
> (defvar allout-mode-map 'allout-mode-map)

i'm doing what you suggest, though i'm also using the
define-minor-mode :keymap argument, since it'll work fine with this
scheme.

>>> This said, you can modify a keymap after the fact: as long as you don't
>>> do a (setq allout-mode-map <newmap>), you can modify allout-mode-map on
>>> the fly and those changes will take effect immediately without having to
>>> use an indirection through the allout-mode-map symbol.
>> are you suggesting doing a rplacd on the minor-mode-map-alist cell?
>
> No, I'm really talking about modifying the keymap itself directly rather
> than modifying various variables that point to it.  E.g. I'm suggesting
> doing a bunch of define-key only.  But if some of your changes involve
> adding/moving bindings it's more difficult, so you could instead use
> (setcdr allout-mode-map (cdr newmap)), but that would be ugly.

in fact, allout enables the user to set the command prefix and any of
the bindings via customization, so a lot of bindings can change at any
time.  (that may be the reason for the fact that allout uses the
'(allout-mode . allout-mode-map) entry form on minor-mode-map-alist
while other minor modes don't, as andreas schwab noticed.)  i agree
that using setcdr this way is ugly, and realize it's not necessary.

> But the above
>
> (defvar allout-mode-map 'allout-mode-map)
>
> should work much more cleanly.

i've adopted this approach instead of fset in my new revision, and it
is cleaner.  i've also unravelled and eliminated a bunch of
accumulated cruft, getting rid of a bunch of code and leaving what
seems to be some necessary complexity, like the defalias, in order to
deal with the way keymaps and define-minor-mode work.

upshot: allout is now migrated to define-minor-mode (as well as epg
instead of pgg, previously), and there's been a housecleaning of a lot
of residual cruft and bugs in the process.  it's not been painless,
but i admit necessary.  thanks for the help in arriving at decent
technical solutions for some of the tricky questions along the way.
let me know if you find objections with what i've done.

ken