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

[ken manheimer]

On Tue, Jan 11, 2011 at 4:42 PM, Stefan Monnier
<address@hidden> wrote:
>> i've tried tracking down the failed key substitutions, by
>> wrapping define-key with some advice that noted when the key defines
>> happened, and they do every time the mode is activated, whether or not it's
>> via the byte-compiled version of the mode function (when defined
>> by define-minor-mode).  my function which does the key
>> substitutions (allout-setup-mode-map) uses fset to ensure that the mode map
>> is properly globally established, so it's already kind of complicated.  i'm
>> also wondering whether i'm just misunderstanding something about the way
>> define-minor-mode is supposed to work.
>
> I think the code is too complex for its own good, so I can't really
> track down the problem.  But AFAICT, the `allout-mode-map' symbol is
> never used as a keymap, so the (fset 'allout-mode-map allout-mode-map)
> has no effect: all the rest of the code uses the allout-mode-map
> *variable* (i.e. the value stored in the `symbol-value' part of the
> allout-mode-map symbol).

i agree that the code is painfully complicated.  however, the fset
actually is effective and necessary to impose the minor-mode keymap
adjustments allout does.

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.  seeing that lead
to the cause of the problem, or at least a workaround.  the workaround
adds more complexity, though.  i'd like to describe the situation to
see if anyone can suggest a better approach.

allout needs to adjust the keymap when the mode is being established,
to take into account some user key binding customizations, of:
`allout-command-prefix', `allout-prefixed-keybindings', and
`allout-unprefixed-keybindings'.  to enable this, i made allout
register its minor-mode-map-alist entry as `(allout-mode .
allout-mode-map)'.  for this format, the keymap facility seeks the key
bindings in allout-mode-map's function value.

the problem with switching to define-minor-mode is that
define-minor-mode apparently associates the mode name with the keymap
value, itself, on minor-mode-map-alist.  thus, the dynamic adjustments
- the customized keymap prefix and key binding substitutions - weren't
taking effect, depending on the order of the redundant entries.

(i assume that ordering variations are why i would not see the problem
when i did a manual eval-defun of the define-minor-mode form, but did
see it when using the byte-compiled definition.)

since i don't control how define-minor-mode populates
minor-mode-map-alist, i have my allout-setup-mode-map routine clear
out all allout-mode entries on minor-mode-map-alist and reestablish
the `(allout-mode . allout-mode-map)' entry.  apparently
define-minor-mode does its keymap arranging early on, so calling
allout-setup-mode-map in the body corrects for the setting and all is
now ok.

except the code is still complicated.  i am open to suggestions about
simpler ways to provide for the customizable key bindings, but in the
meanwhile am checking in the current setup to the trunk.

this would be simpler if define-minor-mode used the `(x-mode .
x-mode-map)' form of entry on minor-mode-map-alist.  that would allow,
in general, for changing the keybindings without removing the
association on minor-mode-map-alist.

oh, yeah, i also migrated the setq stuff to the defvar i already had
established.  i think the reason i used a separate setq was for an
early version of the dynamic keymap settings, and i never rectified
the separation once i revised the approach.  thanks for that pointer,
and thanks much for your attention to my help request, in general!

ken

>        Stefan
>
> PS: While I'm here, don't use `setq' at top-level.  Either put the right
> initial value into `defvar', or if you want that value to be re-set when
> rereading the file, use `defconst' instead of defvar, but don't use
> `setq' at top-level.